Conversation
Could you rename |
Codecov Report
@@ Coverage Diff @@
## master #681 +/- ##
==========================================
- Coverage 72.29% 71.78% -0.51%
==========================================
Files 58 56 -2
Lines 3140 2889 -251
==========================================
- Hits 2270 2074 -196
+ Misses 581 551 -30
+ Partials 289 264 -25
Continue to review full report at Codecov.
|
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
app can only be ran from an image built by docker app build Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
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 + tests ok 👌
internal/commands/run.go
Outdated
Long: longDescription, | ||
Example: example, | ||
Args: cli.RequiresMaxArgs(1), | ||
RunE: func(cmd *cobra.Command, args []string) error { | ||
return runInstall(dockerCli, firstOrEmpty(args), opts) | ||
return runRun(dockerCli, firstOrEmpty(args), opts) |
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.
runRun! 🏃
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.
Shouldn't be firstOrEmpty(args)
I think right? Now the command needs the app image reference?
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.
Mayeb I should rename RunForestRun
?
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.
or RunLolaRun
?
internal/commands/run.go
Outdated
Aliases: []string{"deploy"}, | ||
Short: "Install an application", | ||
Short: "Run an application", | ||
Long: longDescription, | ||
Example: example, | ||
Args: cli.RequiresMaxArgs(1), |
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.
Should be ExactArgs(1)
?
internal/commands/run.go
Outdated
|
||
cmd := &cobra.Command{ | ||
Use: "install [APP_NAME] [--name INSTALLATION_NAME] [--target-context TARGET_CONTEXT] [OPTIONS]", | ||
Use: "run [APP_NAME] [--name INSTALLATION_NAME] [--target-context TARGET_CONTEXT] [OPTIONS]", |
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.
Use: "run [OPTIONS] APP_IMAGE"
? To better fit docker run
synopsis ?
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
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
- What I did
Renamed
install/deploy
command to "run" to align withdocker run
UX- How I did it
refactor > rename
- How to verify it
docker app --help
docker app run myapp
- Description for the changelog
deploy
command has been renamedrun
to align withdocker run
UX- A picture of a cute animal (not mandatory but encouraged)