Add Annotations to command and flags per phase annotation. #2022

tejal29 · 2019-04-25T17:53:26Z

Inspired by @balopat feedback on #2018 , in this change Command have annotations with phases they run like "build", "test", "deploy".

Annotations Description:

Annotation	Description
build	Runs a build
test	Runs tests
deploy	Performs Deploy
cleanup	flags related to clean up
events	uses events api to send events

With this new change, hopefully only relevant flags are added for commands.

Flags Removed or Added per Command

Command	Flags removed	Flags Added	Note
build	--no-prune, --toot
delete	--enable-rpc, --rpc-port, --rpc-int-port, --toot		Events api not used in this flow
deploy	--cache-artifacts , --cache-file,--insecure-registry, --skip-tests
dev		--force (for deploy phase,false by default)

Some considerations:
Maybe toot should be a common flag for build, delete, deploy and run without --tail.

priyawadhwa

just a couple nits

cmd/skaffold/app/cmd/dev.go

cmd/skaffold/app/cmd/flags.go

tejal29 · 2019-04-25T18:07:50Z

TODO: --tail has different defaults in depoy, dev and run.
Fix that

codecov-io · 2019-04-26T02:04:00Z

Codecov Report

Merging #2022 into master will decrease coverage by <.01%.
The diff coverage is 95.74%.

@@            Coverage Diff            @@
##           master   #2022      +/-   ##
=========================================
- Coverage   57.31%   57.3%   -0.01%     
=========================================
  Files         186     187       +1     
  Lines        7871    7875       +4     
=========================================
+ Hits         4511    4513       +2     
- Misses       2948    2949       +1     
- Partials      412     413       +1

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/deploy.go	`68.75% <100%> (ø)`	⬆️
cmd/skaffold/app/cmd/debug.go	`61.53% <100%> (ø)`	⬆️
pkg/skaffold/config/options.go	`92.85% <100%> (ø)`	⬆️
cmd/skaffold/app/cmd/run.go	`60% <100%> (ø)`	⬆️
cmd/skaffold/app/cmd/cmd.go	`65% <100%> (-8.34%)`	⬇️
cmd/skaffold/app/cmd/dev.go	`24.44% <100%> (ø)`	⬆️
cmd/skaffold/app/cmd/delete.go	`66.66% <100%> (+3.03%)`	⬆️
cmd/skaffold/app/cmd/build.go	`69.23% <100%> (+0.8%)`	⬆️
cmd/skaffold/app/cmd/flags.go	`92.59% <92.59%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b7a28e...72f2511. Read the comment docs.

priyawadhwa

just one more question & a nit

cmd/skaffold/app/cmd/flags.go

cmd/skaffold/app/cmd/flags_test.go

tejal29 · 2019-04-29T20:45:40Z

@priyawadhwa Added comment to describe the same.

priyawadhwa

Cool, LGTM

balopat

in general this looks good - but the force flag with default false on dev now gets added - but it wasn't there before! that is a regression on #1484.
This is the second example of a flag that needs to be added with different default depending on the command:

tailFalse for deploy, run
tailTrue for dev

forceFalse for deploy, run
forceTrue for dev

Also I don't understand fully the tail situation yet, it is very hard to follow.

tejal29 · 2019-04-29T22:54:23Z

Great! i feel with "--force" being another example along with "--tail" where defaults are different per command, i should think of a way to specify default behavior based on command.
I will make changes to this PR.

tejal29 · 2019-04-30T18:45:41Z

@priyawadhwa and @balopat please take another look.
I actually ended up moving the annotations to flags now.
This is was something @balopat and I had discussed previously but i veto-ed it :)
Looks like i have to do it that way to add flags with different defaults.

tejal29 · 2019-04-30T20:00:33Z

Freeze for now until @balopat comes back. As per last discussion, he would want to see flagset per flag with annotation for which command they belong to versus right now its a group of flags.

container-tools-bot · 2019-05-22T21:33:19Z

Please visit http://35.236.23.230:1313 to view changes to the docs.

balopat

This is looking very good - only nits

cmd/skaffold/app/cmd/flags.go

balopat · 2019-05-22T21:34:02Z

cmd/skaffold/app/cmd/flags.go

+/// specify "all"
+// FlagAddMethod is method which defines a flag value with specified
+// name, default value, and usage string. e.g. `StringVar`, `BoolVar`
+var FlagResitry = []Flag{


Suggested change

var FlagResitry = []Flag{

var FlagRegistry = []Flag{

pkg/skaffold/config/options.go

balopat · 2019-05-22T21:38:49Z

cmd/skaffold/app/cmd/flags.go

+	},
+	{
+		Name:          "force",
+		Usage:         "Recreate kubernetes resources if necessary for deployment, warning: might cause downtime!)",


Suggested change

Usage: "Recreate kubernetes resources if necessary for deployment, warning: might cause downtime!)",

Usage: "Recreate kubernetes resources if necessary for deployment (default: false, warning: might cause downtime!)",

cmd/skaffold/app/cmd/flags.go

balopat

Yay!

tejal29 requested review from balopat, dgageot, nkubala and priyawadhwa as code owners April 25, 2019 17:53

googlebot added the cla: yes label Apr 25, 2019

priyawadhwa reviewed Apr 25, 2019

View reviewed changes

cmd/skaffold/app/cmd/dev.go Outdated Show resolved Hide resolved

cmd/skaffold/app/cmd/flags.go Outdated Show resolved Hide resolved

tejal29 force-pushed the use_flagset branch 2 times, most recently from 8ccb48c to 414b88a Compare April 26, 2019 01:38

tejal29 force-pushed the use_flagset branch 2 times, most recently from d10f864 to e704e4e Compare April 29, 2019 18:49

priyawadhwa reviewed Apr 29, 2019

View reviewed changes

cmd/skaffold/app/cmd/flags.go Outdated Show resolved Hide resolved

cmd/skaffold/app/cmd/flags_test.go Outdated Show resolved Hide resolved

priyawadhwa approved these changes Apr 29, 2019

View reviewed changes

balopat suggested changes Apr 29, 2019

View reviewed changes

tejal29 force-pushed the use_flagset branch 2 times, most recently from c3d5a42 to a2a99a0 Compare April 30, 2019 18:44

tejal29 added the area/cli label Apr 30, 2019

tejal29 force-pushed the use_flagset branch from a2a99a0 to 16835d0 Compare May 22, 2019 04:53

tejal29 added 7 commits May 21, 2019 21:56

Correct flags except for --tail in dev and deploy

09d16a2

Fixed --tail and removed opts.TailDev

49a7e05

Override tail config

01b22d7

rename debug flags to cleanup

47f85fc

merge from master

4d99b99

Bring TailDev back. Maybe this should be renamed to something else

ec3a257

Regenerate docs and tests after merge with master

8fc4c4e

tejal29 added 4 commits May 21, 2019 21:58

code review comments

67b68bc

another round

c10dc23

rebase from master

4dfc133

Use reflect and custom flag type

60c3058

tejal29 force-pushed the use_flagset branch from 16835d0 to 60c3058 Compare May 22, 2019 16:46

tejal29 added the docs-modifications runs the docs preview service on the given PR label May 22, 2019

container-tools-bot removed the docs-modifications runs the docs preview service on the given PR label May 22, 2019

balopat suggested changes May 22, 2019

View reviewed changes

balopat reviewed May 22, 2019

View reviewed changes

cmd/skaffold/app/cmd/flags.go Outdated Show resolved Hide resolved

balopat approved these changes May 22, 2019

View reviewed changes

fix nits and add comments

72f2511

tejal29 force-pushed the use_flagset branch from 0722b77 to 72f2511 Compare May 22, 2019 22:54

balopat merged commit ab46e0d into GoogleContainerTools:master May 22, 2019

tejal29 deleted the use_flagset branch April 15, 2021 07:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Annotations to command and flags per phase annotation. #2022

Add Annotations to command and flags per phase annotation. #2022

tejal29 commented Apr 25, 2019 •

edited

Loading

priyawadhwa left a comment

tejal29 commented Apr 25, 2019

codecov-io commented Apr 26, 2019 •

edited

Loading

priyawadhwa left a comment

tejal29 commented Apr 29, 2019

priyawadhwa left a comment

balopat left a comment

tejal29 commented Apr 29, 2019 •

edited

Loading

tejal29 commented Apr 30, 2019

tejal29 commented Apr 30, 2019 •

edited

Loading

container-tools-bot commented May 22, 2019

balopat left a comment

balopat May 22, 2019

balopat May 22, 2019 •

edited

Loading

tejal29 May 22, 2019

balopat left a comment

	Usage: "Recreate kubernetes resources if necessary for deployment, warning: might cause downtime!)",
	Usage: "Recreate kubernetes resources if necessary for deployment (default: false, warning: might cause downtime!)",

Add Annotations to command and flags per phase annotation. #2022

Add Annotations to command and flags per phase annotation. #2022

Conversation

tejal29 commented Apr 25, 2019 • edited Loading

priyawadhwa left a comment

Choose a reason for hiding this comment

tejal29 commented Apr 25, 2019

codecov-io commented Apr 26, 2019 • edited Loading

Codecov Report

priyawadhwa left a comment

Choose a reason for hiding this comment

tejal29 commented Apr 29, 2019

priyawadhwa left a comment

Choose a reason for hiding this comment

balopat left a comment

Choose a reason for hiding this comment

tejal29 commented Apr 29, 2019 • edited Loading

tejal29 commented Apr 30, 2019

tejal29 commented Apr 30, 2019 • edited Loading

container-tools-bot commented May 22, 2019

balopat left a comment

Choose a reason for hiding this comment

balopat May 22, 2019

Choose a reason for hiding this comment

balopat May 22, 2019 • edited Loading

Choose a reason for hiding this comment

tejal29 May 22, 2019

Choose a reason for hiding this comment

balopat left a comment

Choose a reason for hiding this comment

tejal29 commented Apr 25, 2019 •

edited

Loading

codecov-io commented Apr 26, 2019 •

edited

Loading

tejal29 commented Apr 29, 2019 •

edited

Loading

tejal29 commented Apr 30, 2019 •

edited

Loading

balopat May 22, 2019 •

edited

Loading