-
Notifications
You must be signed in to change notification settings - Fork 327
cli: Project ops part 1 - CLI updates #2413
Conversation
Pause: there is some weirdness with |
ca40dc9
to
ff5982c
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.
Couple places where we should not corrupt the json output and a TODO.
internal/cli/deployment_list.go
Outdated
@@ -71,6 +71,7 @@ func (c *DeploymentListCommand) Run(args []string) int { | |||
client := c.project.Client() | |||
|
|||
err := c.DoApp(c.Ctx, func(ctx context.Context, app *clientpkg.App) error { | |||
app.UI.Output("%s", app.Ref().Application, terminal.WithHeaderStyle()) |
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.
Same as other about the output not being valid json in this case.
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'm also thinking for the json case, for all projects, instead of returning two separate json structures separated by newlines, we should probably collect and wrap them into a single array for each item listed? Maybe that's a future enhancement 🤔
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.
And if we did that, does it mean single apps are still an item in an array too?
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.
Hm, we could do that by storing the json on the command, returning nothing in the DoApp
if flag.json, and then returning the collected json outside of that loop?
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.
Sure, we could make a more generic mechanism for displaying json where you give the CLI a struct that it will json-ify (and thusly supports multiple structs)
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.
🎉 noice! Looking good. My main issue was getting project/app
targeting for remote projects working again, I think it's just an issue with the new WithMultipleApp
CLI option and it'll work for all commands that use that option.
internal/cli/deployment_list.go
Outdated
@@ -71,6 +71,7 @@ func (c *DeploymentListCommand) Run(args []string) int { | |||
client := c.project.Client() | |||
|
|||
err := c.DoApp(c.Ctx, func(ctx context.Context, app *clientpkg.App) error { | |||
app.UI.Output("%s", app.Ref().Application, terminal.WithHeaderStyle()) |
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'm also thinking for the json case, for all projects, instead of returning two separate json structures separated by newlines, we should probably collect and wrap them into a single array for each item listed? Maybe that's a future enhancement 🤔
002e6b6
to
87a0295
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.
Awesome! Thanks for the updates, this looks legit to me. I found 1 issue I mentioned around raising an error for hostname register <hostname>
if all apps in a project were targeted. Otherwise this looks great! 🎉
(Most of the
Files changed
are updates to all the CLI docs page because of the new-project
flag 😅 )This is a cli-only PR adding a
ProjectTargetRequired
config item and correspondingWithMultipleApp
option to set some commands to target all applications with the targeted project by default. Users can still specify-app=APPNAME
to target a specific app.This change also removes the
-project
flag from thecontext [...]
commands and makes it a global flag. Various other documentation updates are included here as well to amend some of our language from being app-specific to project-specific.This is a surface layer change, as it only supports functionality that we already had on the CLI side, and does not add any project-scoped awareness to our server or job system. It seemed like a good first pass to start with to get the CLI-messaging right.
With this functionality, all commands updated with the new config option will now run that command against each application in the project in sequence. A failure in one app is non-blocking; all applications will finish their jobs individually.
Notable: I moved the
DoApp
ininternal/cli/deployment_destroy.go
from the private func it was in (getDeployments
) up to the parentRun
function; this matches all the other CLI commands and sets us up better to move some of this project-scope to operations later.Example output: