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

bug: remove project resources automatically when delete the project #1421

Conversation

chesedo
Copy link
Contributor

@chesedo chesedo commented Nov 23, 2023

Description of change

This causes a delete project request to try and remove all the resources automatically if possible. Else return a helpful error message.

How has this been tested? (if applicable)

By adding new tests

Copy link
Member

@jonaro00 jonaro00 left a comment

Choose a reason for hiding this comment

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

Dry run flag is not being respected in handler. Ideally, the dry run flag will instead of deleting resources, return a list of them and let the user confirm the deletion of those resources. But we can also just display "...will delete ALL resources!!!" and let it go through after just the initial confirmation in CLI.

Copy link
Contributor

@oddgrd oddgrd 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! I just have a couple of questions.

common/src/models/error.rs Outdated Show resolved Hide resolved
gateway/src/api/latest.rs Outdated Show resolved Hide resolved
gateway/src/api/project_caller.rs Outdated Show resolved Hide resolved
gateway/src/args.rs Show resolved Hide resolved
Copy link
Contributor

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

LGTM! Just left a nit

Comment on lines 99 to 103
if let Some(deployments) = deployments {
Ok(deployments)
} else {
Ok(Vec::new())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: should we use Ok(deployments.unwrap_or_default()? Same in get_resources below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We sure can 😍 Thanks O!

@chesedo chesedo merged commit 5f44ea9 into main Nov 27, 2023
@chesedo chesedo deleted the feature/eng-1882-delete-project-resources-when-delete-request-is-received branch November 27, 2023 17:33
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