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.

Sign up for GitHub

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

Jump to bottom

feat(gateway): special error if own project is already running #1192

Merged
merged 2 commits into from
Sep 1, 2023
Merged

feat(gateway): special error if own project is already running #1192

merged 2 commits into from
Sep 1, 2023

Conversation

thass0
Copy link
Contributor

@thass0 thass0 commented Aug 31, 2023

Description of change

The command cargo shuttle project start fails if a project with the same name is already present. The error message that's printed now tells the caller that their project is already running if they are the project owner.

Closes #1155

How has this been tested? (if applicable)

One assertion has been added to the relevant test for each new case. They make sure that the new error variant ErrorKind::ProjectAlreadyRunning is returned if either the owner or an admin tries to recreate a running project.

@thass0
feat(gateway): special error if own project is already running
5a2c3ab 
The command `cargo shuttle project start` fails if a project
with the same name is already present. The error message that's
printed now tells the caller that their project is already running
if they are the project owner.

Fixes shuttle-hq#1155
@jonaro00
Copy link
Member

My main concern is: What if the project is not in running state? Is it not weird to say "already running" then? I.e. it can be errored, starting, ...
Maybe this can be done by embedding the State (which is not Destroyed) in the new ErrorKind enum variant, and have that state's string representation in the error message. A bit of playing with &str vs String might have to happen.
What do you think?

@thass0
Copy link
Contributor Author

thass0 commented Aug 31, 2023

If we can limit the number of cases, I would prefer not to add any strings to ErrorKind. This will likely increase it's complexity by an unnecessary amount. To begin with, I have to check which additional cases must be considered. The new error message is kind of a shot in the dark. The worst case solution would be to rephrase it as a question if we can't be sure that the project is already running. It could also give a hint on how check the project's state or similar.

jonaro00
jonaro00 approved these changes Aug 31, 2023
View reviewed changes
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.

Alrighty, this will do fine as a first solution.

oddgrd
oddgrd approved these changes Sep 1, 2023
View reviewed changes
Copy link
Contributor

@oddgrd oddgrd left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thanks for adding tests as well! 🤩 Regarding it being a few potential states, we could add "try running cargo shuttle project status" or something similar to the error message. We can merge this either way, though, let me know what you think!

@thass0
Copy link
Contributor Author

thass0 commented Sep 1, 2023

Yes, until we know exactly what cases should be considered, I think a message of that sort would be best. I could add it to this PR later today.

@oddgrd
Copy link
Contributor

oddgrd commented Sep 1, 2023

Cool, sounds good!

@thass0
Copy link
Contributor Author

thass0 commented Sep 1, 2023 •
edited
Loading

How about "it looks like your project is already running. You can find out more with cargo shuttle project status "? This adds the hint to check the project status. The "it looks like" at the start signals that something different might be going on too. We could also omit it for the sake of brevity.

@jonaro00
Copy link
Member

jonaro00 commented Sep 1, 2023

That sounds fine.

@jonaro00 jonaro00 merged commit 5a66ca5 into shuttle-hq:main Sep 1, 2023
@thass0
Copy link
Contributor Author

thass0 commented Sep 1, 2023 •
edited
Loading

I gathered that cargo shuttle project status does nothing except printing the name and the state returned from gateway (based on cargo-shuttle's Shuttle::project_status and the Response and State in common/src/models/projects.rs).

It seems to me that in GatewayService::create_project (where the new error is created), all of this information is available too, since the database query returns the Project that's running at the moment.

If I am correct, we could make GatewayService::create_project generate an error message that basically already contains all of the information that a user would get by running cargo shuttle project status. I would be happy to do so if that's desirable.

@jonaro00
Copy link
Member

jonaro00 commented Sep 1, 2023

Yeah, that was roughly what I was thinking at first. A great task for a follow-up PR! 😃

thass0 added a commit to thass0/shuttle that referenced this pull request Sep 2, 2023
@thass0
feat(gateway): inform project owner about running state
0b8b668 
Now, if `cargo shuttle project start` fails because the
existing project is owned by the caller, the information
from `cargo shuttle project status` is automatically displayed.
Depending on the running state, different hints are provided.

Extends shuttle-hq#1192
thass0 added a commit to thass0/shuttle that referenced this pull request Sep 2, 2023
@thass0
feat(gateway): inform project owner about running state
907b65f 
Now, if `cargo shuttle project start` fails because the
existing project is owned by the caller, the information
from `cargo shuttle project status` is automatically displayed.
Depending on the running state, different hints are provided.

Extends shuttle-hq#1192
@thass0 thass0 mentioned this pull request Sep 2, 2023
Merged
jonaro00 pushed a commit that referenced this pull request Sep 5, 2023
@thass0
feat(gateway): inform project owner about running state (#1194)
2fa1db3 
* feat(gateway): inform project owner about running state

Now, if `cargo shuttle project start` fails because the
existing project is owned by the caller, the information
from `cargo shuttle project status` is automatically displayed.
Depending on the running state, different hints are provided.

Extends #1192

* Improve PR #1194
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonaro00 jonaro00 jonaro00 approved these changes

@oddgrd oddgrd oddgrd approved these changes

@orhun orhun orhun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Improvement]: Correct behavior when running project start
4 participants
@thass0 @jonaro00 @oddgrd @orhun