Skip to content
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

[DC-1205]Store TDR owned Sam Group info in database #1788

Merged
merged 21 commits into from
Aug 27, 2024

Conversation

rjohanek
Copy link
Contributor

@rjohanek rjohanek commented Aug 21, 2024

Jira ticket: https://broadworkbench.atlassian.net/browse/DC-1205

Summary of changes

  1. Database change
    • Add columns to the snapshot_request table: sam_group_name, sam_group_created_by
  2. Models changes
    • Add samGroupName and samGroupCreatedBy to SnapshotAcessRequestModel
    • Add samGroupName to SnapshotAcessRequestResponse
  3. SnapshotCreateFlight change
    • Update AddCreatedSnapshotIdToSnapshotRequestStep -> AddCreatedInfoToSnapshotRequestStep to save not just the created snapshot id but also the sam group name and the tdr service account that created the group
    • Update snapshotRequestDao method to update created snapshot id, sam group name, and sam group created by in one method
  4. SnapshotDeleteFlight change
    • Update the DeleteSnapshotDeleteSamGroupStep, instead of reconstructing the sam group name, retrieve it from the request that led to the creation of the snapshot currently being deleted.
    • Add snapshotRequestDao method getByCreatedSnapshotId. This was needed for the DeleteSnapshotDeleteSamGroupStep to get the sam group from the request.
    • Modify DeleteSnapshotPopAndLockDatasetStep to remove saving the authdomains in the map, also remove the auth domain groups map key now that it is unused

Testing Strategy

  1. Unit tests added/updated for the two steps updated, the two new dao methods, and the create flight.
  2. Additionally, the delete functionality is already tested in the integration and connected test.
  3. Add tests for doStep for DeleteSnapshotPopAndLockDatasetStep

…ed on the snapshot access request that created the snapshot
# Conflicts:
#	src/main/java/bio/terra/service/snapshot/flight/create/SnapshotCreateFlight.java
#	src/test/java/bio/terra/service/snapshot/SnapshotServiceTest.java
#	src/test/java/bio/terra/service/snapshot/flight/create/SnapshotCreateFlightTest.java
#	src/test/java/bio/terra/service/snapshotbuilder/SnapshotBuilderServiceTest.java
@rjohanek rjohanek requested review from a team as code owners August 21, 2024 18:39
@rjohanek rjohanek requested review from rushtong and okotsopoulos and removed request for a team August 21, 2024 18:39
Copy link
Member

@pshapiro4broad pshapiro4broad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost done, still looking at the test code

@rushtong rushtong removed their request for review August 26, 2024 15:29
Copy link
Member

@pshapiro4broad pshapiro4broad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, one last comment or two

Copy link
Contributor

@snf2ye snf2ye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Phil's last couple of comments, but otherwise LGTM.

@rjohanek rjohanek enabled auto-merge (squash) August 26, 2024 19:54
Copy link

@rjohanek rjohanek merged commit 9d145a2 into develop Aug 27, 2024
14 checks passed
@rjohanek rjohanek deleted the rj/dc-1205-add-collaborators-1 branch August 27, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants