-
Notifications
You must be signed in to change notification settings - Fork 258
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
feat(gateway): block project deletion on secrets deletion #1338
feat(gateway): block project deletion on secrets deletion #1338
Conversation
gateway/src/api/latest.rs
Outdated
) | ||
}) | ||
.map(|resource| resource.r#type.to_string()) | ||
.collect::<HashSet<_>>() |
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.
This may be a bit overkill (it needs to add a few Derive(Hash)
annotations), there shouldn't be more than one of each resource, but we have seen that it can happen.
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.
In that case, it might be better to not dedup them here, so that the issue is not obfuscated.
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.
This may be a bit overkill (it needs to add a few
Derive(Hash)
annotations), there shouldn't be more than one of each resource, but we have seen that it can happen.
Not sure why it matters if there is more than one resource attached for checking if the project has resources.
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'll drop it, thanks!
d597007
to
faf1961
Compare
gateway/src/api/latest.rs
Outdated
) | ||
}) | ||
.map(|resource| resource.r#type.to_string()) | ||
.collect::<HashSet<_>>() |
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.
In that case, it might be better to not dedup them here, so that the issue is not obfuscated.
gateway/src/api/latest.rs
Outdated
) | ||
}) | ||
.map(|resource| resource.r#type.to_string()) | ||
.collect::<HashSet<_>>() |
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.
This may be a bit overkill (it needs to add a few
Derive(Hash)
annotations), there shouldn't be more than one of each resource, but we have seen that it can happen.
Not sure why it matters if there is more than one resource attached for checking if the project has resources.
13a9e85
to
51f20c8
Compare
* feat(cargo-shuttle): project delete command * fix(cargo-shuttle): Refactor spinner, fix spam on service stop command * feat(gateway): delete container and volume, delete from db, bump bollard, layout for gateway endpoint * feat(gateway): add checks before project delete. fix stopping of running deployment. * feat(gateway): use status endpoint, check for 404's, delete custom domains, fix stop command (caveat in testcase) * fix(provisioner): validate project name * fix(provisioner): gracefully handle deletion of non-existent db * feat(gateway): project delete sqlx transaction * test(gateway service): add basic project delete tests * feat(gateway): block project delete on db or secrets (#1338) --------- Co-authored-by: Iulian Barbu <iulianbarbu2@gmail.com> Co-authored-by: Oddbjørn Grødem <29732646+oddgrd@users.noreply.github.com>
Description of change
This is based on #1307.
This forces users to delete secrets from resource-recorder before they can delete a project.
How has this been tested? (if applicable)
With a local run of a project with secrets and postgres.