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

feat: suggest project restart when trying to delete #1366

Merged
merged 2 commits into from Nov 8, 2023

Conversation

jonaro00
Copy link
Member

@jonaro00 jonaro00 commented Nov 3, 2023

Description of change

Additional context when project is not ready on delete command.
image

How has this been tested? (if applicable)

See image above

client.delete_project(self.ctx.project_name(), true).await.map_err(|err| {
if let Some(api_error) = err.downcast_ref::<ApiError>() {
// If the returned error string changes, this could break
if api_error.message.contains("not ready") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we should use plain string match or have a constant somewhere for the message and check that.

e.g.

const PROJECT_NOT_READY_MSG: &str =  "project not ready. Try running `cargo shuttle project restart`.";

It is something trivial but worth considering I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

I used a small string for matching, because if an entire string is used like that, a single change to the string will be breaking, like we saw in #1360. In this scenario, it wouldn't break functionality, just the extra hint, but still.

@oddgrd oddgrd merged commit 3f14217 into shuttle-hq:main Nov 8, 2023
33 of 34 checks passed
@jonaro00 jonaro00 deleted the delete-restart branch November 8, 2023 19:05
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.

None yet

3 participants