-
Notifications
You must be signed in to change notification settings - Fork 174
Filter deleted resources in get_by_id repository methods #4793
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Updated get_workspace_by_id to exclude deleted workspaces - Updated get_workspace_service_by_id to exclude deleted services - Updated get_user_resource_by_id to exclude deleted user resources - Updated get_shared_service_by_id to exclude deleted shared services - Added/updated unit tests to verify deleted resources are filtered - Updated CHANGELOG.md with bug fix entry Co-authored-by: marrobi <17089773+marrobi@users.noreply.github.com>
api_app/tests_ma/test_db/test_repositories/test_shared_service_repository.py
Fixed
Show fixed
Hide fixed
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Unit Test Results668 tests 668 ✅ 7s ⏱️ Results for commit f73fac4. ♻️ This comment has been updated with latest results. |
|
@copilot update the API version |
Incremented API version from 0.25.6 to 0.25.7 for bug fix release. Added version update entry to CHANGELOG.md COMPONENTS section. Co-authored-by: marrobi <17089773+marrobi@users.noreply.github.com>
|
api version will need upping again |
…ithub.com/microsoft/AzureTRE into copilot/fix-deleted-workspaces-url-access
JC-wk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested this and deleted workspaces are no longer visible @marrobi
|
@tamirkamara are you able to look at this one? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marrobi I believe something not right here as the E2E consistently fail to delete workspaces.
|
Ah, I know what it is. The operation is giving 404 as the service no longer exists. The tests need updating to accept a 404 as its been deleted. I thought had fixed this. Maybe not pushed. |
|
@copilot The e2e tests in this pr fail as the operations endpoint returns 404. Once the workspace services and user resource are deleted it cannot be accessed. 404 should be a valid response that the resource has been deleted. Please fix this. |
When checking operation status, if the parent resource has been deleted, the operations endpoint will return 404 (due to the get_by_id filtering). This is now treated as a valid deleted state rather than an error. Also incremented API version to 0.25.12. Co-authored-by: marrobi <17089773+marrobi@users.noreply.github.com>
Fixed in commit ba01e4a. The
Updated API version to 0.25.12 in commit ba01e4a |
|
/test-extended ba01e4a |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/21144706377 (with refid (in response to this comment from @marrobi) |
|
@tamirkamara can you take another look? |
The COMPONENTS section is auto-generated during releases and should not be manually updated in PRs per copilot-instructions.md. Co-authored-by: tamirkamara <26870601+tamirkamara@users.noreply.github.com>
The E2E test fix is an implementation detail required to support the main bug fix (filtering deleted resources), not a separate user-facing bug fix. Co-authored-by: tamirkamara <26870601+tamirkamara@users.noreply.github.com>
|
/test-force-approve |
|
🤖 pr-bot 🤖 ✅ Marking tests as complete (for commit f73fac4) (in response to this comment from @tamirkamara) |
What is being addressed
The
get_*_by_idmethods in repository classes return deleted resources, while list methods filter them out. This allows accessing deleted workspaces, workspace services, user resources, and shared services via direct URL after deletion.How is this addressed
Added
deploymentStatus != Status.Deletedfilter to allget_by_idqueries:WorkspaceRepository.get_workspace_by_id()WorkspaceServiceRepository.get_workspace_service_by_id()UserResourceRepository.get_user_resource_by_id()SharedServiceRepository.get_shared_service_by_id()Query pattern:
Updated E2E tests to support the filtering changes:
check_deployment()ine2e_tests/resources/deployment.pyto treat 404 status as a valid deleted stateUpdated unit tests to verify deleted resources raise
EntityDoesNotExist.Updated CHANGELOG.md with bug fix entry.
Incremented API version to 0.25.12 in
api_app/_version.py.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.