Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add/expose cli flags #771

Merged
merged 6 commits into from
Apr 2, 2023
Merged

feat: add/expose cli flags #771

merged 6 commits into from
Apr 2, 2023

Conversation

hans-m-song
Copy link
Contributor

@hans-m-song hans-m-song commented Mar 30, 2023 •
edited
Loading

Every time I execute commands, I would like to opt out of force-updates

This adds additional flags to the cli:

  • --skip-deps - hoisted up to the global level
  • --disable-force-update - exposes the ability to disable the force-update flag

Resolves roboll/helmfile/issues/795
Relates to roboll/helmfile#1494 (review)

Still a rookie at go (especially a repo of this size), any feedback is appreciated

@hans-m-song hans-m-song force-pushed the feat/expose-cli-flags branch from e980174 to 23753d2 Compare March 30, 2023 13:16
@hans-m-song
feat: add/expose cli flags
3f7c79a 
Signed-off-by: Hans Song <hans.m.song@gmail.com>
@hans-m-song hans-m-song force-pushed the feat/expose-cli-flags branch from 23753d2 to 3f7c79a Compare March 30, 2023 13:16
@hans-m-song
fix tests
cb3dee4 
Signed-off-by: Hans Song <hans.m.song@gmail.com>
@hans-m-song hans-m-song force-pushed the feat/expose-cli-flags branch from f57f5bf to cb3dee4 Compare March 30, 2023 23:11
@yxxhero
Copy link
Member

yxxhero commented Mar 31, 2023

@hans-m-song Thanks for your PR. if it's ready. I will review that and give some feedback.

@yxxhero yxxhero marked this pull request as ready for review March 31, 2023 10:20
@yxxhero yxxhero marked this pull request as draft March 31, 2023 10:20
@hans-m-song hans-m-song marked this pull request as ready for review March 31, 2023 11:16
@hans-m-song
Copy link
Contributor Author

thanks @yxxhero, looking forward to it 😄

yxxhero
yxxhero reviewed Apr 1, 2023
View reviewed changes
pkg/config/apply.go Outdated
@@ -146,7 +146,7 @@ func (a *ApplyImpl) SkipCleanup() bool {

// SkipDeps returns the skip deps.
func (a *ApplyImpl) SkipDeps() bool {
return a.ApplyOptions.SkipDeps
return a.GlobalOptions.SkipDeps || a.ApplyOptions.SkipDeps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove SkipDeps field in ApplyOptions. others are similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for my understanding... this is because ApplyImpl embeds GlobalImpl which embeds GlobalOptions?

@hans-m-song
remove skipdeps from subcommand options
ac456ea 
Signed-off-by: Hans Song <hans.m.song@gmail.com>
@yxxhero
Copy link
Member

yxxhero commented Apr 1, 2023

@hans-m-song ah. you got me.

@yxxhero
Copy link
Member

yxxhero commented Apr 1, 2023 •
edited
Loading

@hans-m-song you can refer to the logic of --environment.

helmfile/cmd/root.go

Line 119 in fe1eba6

fs.StringVarP(&globalOptions.Environment, "environment", "e", "", `specify the environment name. defaults to "default"`)

@hans-m-song
Copy link
Contributor Author

@yxxhero sorry not sure I follow 🤔

--skip-Deps and --disable-force-update are registered in root where you have linked to --environment

https://github.com/helmfile/helmfile/pull/771/files#diff-ab967ab1a2f3a1b769106eeb7bfe892ef0e81d1d27811fa15be08e6749feee1fR122-R123

DisableForceUpdate is accessible in GlobalImpl similar to how Environment is with Env(), are you saying I should expose SkipDeps in the same way?

https://github.com/helmfile/helmfile/pull/771/files#diff-3ec397d110aac0e46127983382e5ef242ff19068d7390217684559aaeff8a364R139-R142

@yxxhero
Copy link
Member

yxxhero commented Apr 2, 2023

@hans-m-song if added in global level. then other subcommands don't need to add.

@hans-m-song
remove skip-deps from subcommand flags
41e32b3 
Signed-off-by: Hans Song <hans.m.song@gmail.com>
yxxhero
yxxhero reviewed Apr 2, 2023
View reviewed changes
pkg/config/global.go
@@ -132,6 +136,11 @@ func (g *GlobalImpl) EnableLiveOutput() bool {
return g.GlobalOptions.EnableLiveOutput
}

// DisableForceUpdate return when to disable forcing updates to repos upon adding
func (g *GlobalImpl) DisableForceUpdate() bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add SkipDeps function. remote in other subcommands.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hans-m-song

@hans-m-song
remove SkipDeps from subcommand implementations
d60cecb 
Signed-off-by: Hans Song <hans.m.song@gmail.com>
@yxxhero
Copy link
Member

yxxhero commented Apr 2, 2023

@hans-m-song final step. please update the docs: https://github.com/helmfile/helmfile/blob/main/docs/index.md#cli-reference

@hans-m-song
update doco with new flags
c1ae0dc 
Signed-off-by: Hans Song <hans.m.song@gmail.com>
@yxxhero yxxhero merged commit 1d0ba72 into helmfile:main Apr 2, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@yxxhero yxxhero yxxhero approved these changes

@mumoshu mumoshu Awaiting requested review from mumoshu mumoshu is a code owner

@itscaro itscaro Awaiting requested review from itscaro itscaro is a code owner

@xiaomudk xiaomudk Awaiting requested review from xiaomudk xiaomudk is a code owner

Assignees
No one assigned
Labels
feature in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ability to configure --skip-deps globally
2 participants
@hans-m-song @yxxhero