-
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 apps subcommand that allows to list and delete apps #67
Conversation
4ea8eb7
to
abb0530
Compare
src/commands/apps.rs
Outdated
let mut app_list_page = client.list_apps(DEFAULT_APPLIST_PAGE_SIZE, None).await?; | ||
match (app_list_page.is_last_page, cmd.all) { | ||
(true, _) | (false, true) => println!("Apps ({})", app_list_page.total_items), | ||
(false, false) => println!("Note: Displaying the first {} of {} Apps. To list all the apps run \"spin cloud apps list -all\"", DEFAULT_APPLIST_PAGE_SIZE, app_list_page.total_items), |
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.
"all" should be the default (if not the only option). Showing one page is effectively giving the user a random subset, which doesn't seem terribly useful, or am I missing something? I guess maybe if all they care about is the count...?
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 like making it the default option - I will add an option called count that returns the total number of apps.
src/commands/deploy.rs
Outdated
@@ -404,7 +404,7 @@ impl DeployCommand { | |||
cloud_client: &CloudClient, | |||
name: String, | |||
) -> Result<Option<Uuid>> { | |||
let apps_vm = CloudClient::list_apps(cloud_client).await?; | |||
let apps_vm = CloudClient::list_apps(cloud_client, 50, None).await?; |
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.
Maybe out of scope but it feels like this should page too - what is the app is not in the first page?
src/commands/login.rs
Outdated
@@ -230,7 +231,7 @@ impl LoginCommand { | |||
insecure: self.insecure, | |||
token: token.clone(), | |||
}) | |||
.list_apps() | |||
.list_apps(DEFAULT_APPLIST_PAGE_SIZE, None) |
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.
Maybe out of scope, but I believe the page size here could be 1 (since this is just to verify that the creds work, not to actually list any apps).
@@ -19,10 +20,21 @@ pub(crate) async fn create_cloud_client(deployment_env_id: Option<&str>) -> Resu | |||
} | |||
|
|||
pub(crate) async fn get_app_id_cloud(cloud_client: &CloudClient, name: &str) -> Result<Uuid> { | |||
let apps_vm = CloudClient::list_apps(cloud_client).await?; | |||
let apps_vm = CloudClient::list_apps(cloud_client, DEFAULT_APPLIST_PAGE_SIZE, None).await?; |
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.
Again, this needs to page (but may be out of scope for this PR)
api_apps_get(&self.configuration, None, None, None, None, None, None) | ||
.await | ||
.map_err(format_response_error) | ||
pub async fn list_apps(&self, page_size: i32, page_index: Option<i32>) -> Result<AppItemPage> { |
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.
page_size
and page_index
should really be unsigned at the Rust level even if .NET advertises it as signed. (But I get this runs into issues about "what if somebody asks for 2^31 + 1 items how do you convert that Ivan".)
a0ba904
to
737242a
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.
The only blocker is the incorrect syntax for the count flag, but I think we should also make sure we have consensus on the UI e.g. whether we want to have the count flag, whether we want to have a header row, and whether we want to print anything when there are no entities. This is important because of likely broader implications e.g. we'd probably want the sqlite list
command to have the same behaviour.
737242a
to
f2a72e2
Compare
src/commands/apps.rs
Outdated
AppsCommand::List(cmd) => { | ||
let client = create_cloud_client(cmd.common.deployment_env_id.as_deref()).await?; | ||
let mut app_list_page = client.list_apps(DEFAULT_APPLIST_PAGE_SIZE, None).await?; | ||
match app_list_page.total_items > 0 { |
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.
Curious, why did you choose to do a match statement here instead of an if statement?
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.
Past reviews have trained me to prefer match
over if
but I do not know if it is not appropriate here.
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 would usually only match on a bool if it was evaluating to a result that the match was capturing. I'm curious if there is a best practice here. @itowlson any thoughts?
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.
For a predicate like this I'd probably choose if
because it avoids the clutter of the true => / false =>
prefixes (but my opinion is not strong, and I am a really poor bellwether for what the Rust Gods consider best practice).
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 think this is looking ready to switch from Draft to Ready for Review. To your comment @itowlson, I am aligned with no headers and instead output on empty as is done here.
f2a72e2
to
b546bd6
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, one non-blocking suggestion (plus the comment about if vs match which is also non-blocking)
impl AppsCommand { | ||
pub async fn run(self) -> Result<()> { | ||
match self { | ||
AppsCommand::List(cmd) => { |
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.
Style suggestion: In many places we move each leaf command's run
implementation down to that leaf command, so the intermediate level becomes (cf. https://github.com/fermyon/spin/blob/60011faf310e673f20fc7d5de20f4df8a99374e7/src/commands/plugins.rs#L43)
impl FieCommand {
fn run(&self) {
match self {
Self::AliceCommand(cmd) => cmd.run(),
Self::BobCommand(cmd) => cmd.run(),
}
}
}
This removes a level of structure and indentation from the actual real command logic, and keeps the implementations of the subcommands fully distinct in the source code.
It's not a must-do but it's a pattern I find significantly clearer.
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 based it off the structure in variables
and sqlite
. I do not have a preference here. I am happy to go eitherway. Let me know!
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.
Oh, if cloud-plugin
is inlining everything then I guess be consistent.
@kate-goldenring @itowlson any strong opinions on |
@karthik2804 I am Team If in this case |
Signed-off-by: karthik2804 <karthik.ganeshram@fermyon.com>
b546bd6
to
fda851b
Compare
@itowlson Well I will take the side with Team if this time around then! |
This PR adds a new top-level command
apps
that further haslist
anddelete
subcommands.fixes #9
fixes #40