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-1116] Move from set_parent to create_with_parent #1450

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

s-rubenstein
Copy link
Contributor

Ticket: https://broadworkbench.atlassian.net/browse/DC-1116
<Don't forget to include the ticket number in the PR title!>

What:

<For your reviewers' sake, please describe in a sentence or two what this PR is accomplishing (usually from the users' perspective, but not necessarily).>

Why:

<For your reviewers' sake, please describe in ~1 paragraph what the value of this PR is to our users or to ourselves.>

How:

<For your reviewers' sake, please describe in ~1 paragraph how this PR accomplishes its goal.>

<If the PR is big, please indicate where a reviewer should start reading it (i.e. which file or function).>


PR checklist

  • I've followed the instructions if I've made any changes to the API, especially if they're breaking changes
  • I've filled out the Security Risk Assessment (requires Broad Internal network access) and attached the result to the JIRA ticket

Copy link

sonarqubecloud bot commented Jun 3, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link
Contributor

@rjohanek rjohanek left a comment

Choose a reason for hiding this comment

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

this looks good but how can we test this change?

@s-rubenstein
Copy link
Contributor Author

@rjohanek I think the only real way to test it is to spin up a BEE? Which has proven somewhat difficult, but I think might be possible.

Alternatively, the rollback for this is very fast, and it should be easy to test post merge - I think that the real question is whether there is testing for the create_with_parent action, and that is probably out of scope. (and likely already done by identiteam when they released the feature)

@s-rubenstein
Copy link
Contributor Author

Actually, I could maybe spin up a local Sam and try creating a request resource with a parent, just using the resources APIs. I might try that rather than fiddling with bees

Copy link
Contributor

@rjohanek rjohanek left a comment

Choose a reason for hiding this comment

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

looks good, proposed plan for testing sounds good!

@s-rubenstein
Copy link
Contributor Author

I tested by locally running Sam, creating a datasnapshot resource by hand, and then creating a snapshot-access-request resource by hand with that snapshot resource as the parent.

@s-rubenstein s-rubenstein merged commit 532964c into develop Jun 3, 2024
24 checks passed
@s-rubenstein s-rubenstein deleted the sr/dc-1116-create-with-parent branch June 3, 2024 18:12
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.

3 participants