-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add get app subcommand to get app details #94
Conversation
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.
The blocking one is the blank Description thing; the others are merely suggestions
src/commands/apps.rs
Outdated
@@ -11,6 +11,8 @@ pub enum AppsCommand { | |||
List(ListCommand), | |||
/// Delete an app deployed in Fermyon Cloud | |||
Delete(DeleteCommand), | |||
/// Get Details about a deployed app in Fermyon Cloud | |||
Get(GetCommand), |
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.
Get
seems not quite the right name to me. What about Describe
or Info
? Maybe Get
is right here. Is there a CLI you've seen that you're modeling after?
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 was just using the HTTP method here and it looked like it was descriptive enough of "get" app info - I am open to suggestions here.
One possibility might be get-info
?
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.
Why get-info
over just info
? Maybe we take a vote in chat? I feel like to me, the following is displaying info, and spin cloud apps info "lottery"
feels clearer. "get" like you mention is informative to HTTP method types but IMO CLIs require another level of specificity.
$ spin cloud apps get "lottery"
Name: lottery
Description: Draw a Magic 8 Ball Winner
URL: https://workshoplottery.fermyon.app
4fb7f0e
to
e5119f9
Compare
@itowlson I have addressed the comments. |
e5119f9
to
ea5d3d4
Compare
Signed-off-by: karthik2804 <karthik.ganeshram@fermyon.com>
ea5d3d4
to
423ae7f
Compare
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.
LGTM subject to agreement with Kate on the command name - thanks!
match val.validation_status { | ||
DomainValidationStatus::InProgress => { | ||
if let Some(url) = app.channels[0].domain.as_ref() { | ||
println!("URL: https://{}", url); |
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.
wow, I let one of my error messages stray into column 81, and cargo fmt
gets mediaeval on my code with a rusty fork, but you have 9 levels of indent and it's all "right this way Mister Karthik can I escort you to the VIP zone". THE INJUSTICE
More seriously... I am appreciating the additional refinement of the UI, but the downside is it has made this function very long and very deeply indented. I'd strongly encourage breaking it up - ideally into command handlers, but at least pulling out some of the analysis and print logic into helpers. It's not a blocker though.
Signed-off-by: karthik2804 <karthik.ganeshram@fermyon.com>
@kate-goldenring I have renamed the command to |
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.
Thanks Karthik!
This PR adds a get subcommand to apps to allow for getting details about an app. It helps with getting the Base URL of apps quickly without having to open up the cloud UI.