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.

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

Append --force-update for specific helm versions. #1494

Merged
merged 3 commits into from Oct 12, 2020

Conversation

wi1dcard
Copy link
Contributor

@wi1dcard wi1dcard commented Sep 22, 2020

This PR contains 1 bug fix and 1 change:

@wi1dcard wi1dcard changed the title Parse and process helm version using github.com/Masterminds/semver/v3. Append --force-update for specific helm versions. Sep 22, 2020
@voron
Copy link
Contributor

voron commented Sep 22, 2020

As @RothAndrew mentioned here, it may be a valid case to use --force-update with helm versions 3.3.4+ too, when you're really need to update repository via helmfile. My propose is to get following syntax in helmfile:

helmDefaults:
  repoForceUpdate: false
...
repositories:
  - name: git-based-repo
    url: git+ssh://git@github.com/roboll/helmfile@?ref=<FREQUENTLY_CHANGED_OR_DYNAMIC_REF>
    forceUpdate: true

WDYT?
We don't need this option with helm <3.3.2, but with 3.3.2+ (and 3.3.4+ too) it may be better to let helmfile user decide what to do with it.

@wi1dcard
Copy link
Contributor Author

Hi @voron , I didn't realized that we can use a URL with the specific ref of version. Hmmm, I think that would be a great case of showing why we need to leave the option and let the users decide. What about the default value of repositories.*.forceUpdate? We may have 3 choices:

  1. The default value of repositories.*.forceUpdate is always false.
  2. The default value is determined based on the Helm version, >=3.3.2 && <3.3.4 -> true, otherwise -> false.
  3. The default value is determined based on the Helm version, >=3.3.2 -> true, otherwise -> false.
  4. The default value of repositories.*.forceUpdate is helmDefaults.repoForceUpdate, and the default value of latter one can be one of above.

Personally I prefer 4th + 1st or just the third one. Because:

With 4th + 1st, we keep the default behavior as secure as possible without adding any additional flags, and the users who use the specific helm versions can simplely change the behavior for all repos in one place helmDefaults.repoForceUpdate.

With the third, we can add the --force-update under the hood and do not have to "break" anything. Users won't notice that there's any change either using helm < 3.3.2 or helm > 3.3.4.

What do you think?

cc @mumoshu

@voron
Copy link
Contributor

voron commented Sep 22, 2020

I vote for 4th+1st:

The default value of repositories.*.forceUpdate is helmDefaults.repoForceUpdate
The default value of helmDefaults.repoForceUpdate is false

It's helm "responsibility" to break this thing, I don't see reasons to hide it from helmfile user. We need to provide an ability to adjust this option when required, w/o any "smart" staff IMHO.

@mumoshu
Copy link
Collaborator

mumoshu commented Oct 12, 2020

I vote for 4th + 1st, too. I thought 4th + 2nd or 3rd would be the best before seeing helm/helm#8777 - apparently the Helm team "fixed" the regression issue very quickly 👍

Copy link
Collaborator

@mumoshu mumoshu left a comment

Choose a reason for hiding this comment

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

This LGTM as its current state and I'm merging this as it only enhances the current implementation. Thank you so much for your continued effort @wi1dcard!

I'll leave the implementation of the ability to expose --force-update configurable to another PR(s).

@mumoshu mumoshu merged commit 5d8eba9 into roboll:master Oct 12, 2020
hans-m-song added a commit to hans-m-song/helmfile that referenced this pull request Mar 30, 2023
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/pull/1494#pullrequestreview-506233792

Still a rookie at go (especially of this size), any feedback is appreciated
hans-m-song added a commit to hans-m-song/helmfile that referenced this pull request Mar 30, 2023
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/pull/1494#pullrequestreview-506233792

Still a rookie at go (especially of this size), any feedback is appreciated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants