feat(#344): add sub helmfiles explicit selectors #567

sgandon · 2019-04-27T17:20:18Z

Fixes #344 by allowing explicit selectors to be specified for composed helmfiles using the following structure

helmfiles:
- helmfile.d/a*.yaml:
    selectors:
    - name=prometheus      
    - name!=zipkin      
- helmfile.d/b*.yaml
- helmfile.d/c*.yaml:
    selectors: {}

2 modes here :

legacy mode when no the env var EXPERIMENTAL is not set to true
- no selector : inherit from the command line.
- selector: is specified then it is used (an emty means no inheritance from command line and take everything).
experimental when the env var EXPERIMENTAL=true
- no selector : nothing is inherited from the command line so use all releases.
- selector: is specified then it is used (an emty means no inheritance from command line and take everything).

sgandon · 2019-04-27T17:21:18Z

I forgot to update the readme, I'll do so soon

davidovich · 2019-04-27T17:35:03Z

This seems good. I also like the EXPERIMENTAL flag, but having only one Boolean value feels like it has only one purpose. What about using an identifier like EXPERIMENTAL=selectors ? That way the env var can be used to introduce simultaneous experimental features, controlled by one flag (probably allowing coma separated identifiers).

sgandon · 2019-04-28T08:17:50Z

Year that's a good idea.
I'll change it to a feature enablement flag.

sgandon · 2019-04-28T10:52:43Z

in fact I like the idea of enabling all experimental feature at once, so I guess I'll provide both identifiers and a "true" value to enable them all.

sgandon · 2019-04-29T23:14:34Z

all done, waiting for review.

davidovich · 2019-04-30T00:58:26Z

README.md

@@ -805,6 +826,12 @@ Once you download all required charts into your machine, you can run `helmfile c
 It basically run only `helm upgrade --install` with your already-downloaded charts, hence no Internet connection is required.
 See #155 for more information on this topic.

+## Experimental features
+Some experimental features may be available for testing in perspective of being (or not) included in a future release.
+Those features are set using the environment variable `EXPERIMENTAL`. Here is the current experimental feature :


Nice. As I read this though, I think that EXPERIMENTAL alone cries for name clashes. Can we prefix with HELMFILE_ to remove this potential clash?

absolutly !

mumoshu

Thanks for your contribution!

Looks awesome overall, but if I can wish one thing, I would love the sub-helmfile path to be denoted by the dedicated path setting, rather than the dictionary key.

That would make it extra clear that you can have only one path per sub-helmfile.

helmfiles:
- path: helmfile.d/a*.yaml
  selectors:
  - name=prometheus      
  - name!=zipkin      
- helmfile.d/b*.yaml
- path: helmfile.d/c*.yaml
  selectors: {}

sgandon · 2019-05-02T11:54:33Z

@mumoshu but if I do this (which would have been much easier for parsing) we will loose the backward compatibily that I did maintain with the current PR, or am I missing something ?

sgandon · 2019-05-02T11:55:34Z

or do think this is not an issue to maintain backward compatiblity ?

mumoshu · 2019-05-02T12:14:35Z

@sgandon I do want to keep backward-compatibility, and it looked possible as your implementation is handling it in https://github.com/roboll/helmfile/pull/567/files#diff-f6be34387a141f652301c16cf7a5e36cR1411

That being said, the type switch there wouldn't change.

In my example, the first and the third sub-helmfile should be handled with the map case, and the second case with the string case(backward compatibility):

helmfiles:
- path: helmfile.d/a*.yaml
  selectors:
  - name=prometheus      
  - name!=zipkin      
- helmfile.d/b*.yaml
- path: helmfile.d/c*.yaml
  selectors: {}

sgandon · 2019-05-02T12:47:59Z

ha sorry I did not see this example, so yes we can have both :)
I'll do the change and fix the conflictings files.

sgandon · 2019-05-04T21:24:09Z

@mumoshu could you help me out with the test failure on circle-ci please ? the command make test works fine on my machine.

sgandon · 2019-05-04T21:48:39Z

I believe I found the issue, map range does not guaranty the order. I'll fix that.

mumoshu · 2019-05-05T04:30:49Z

state/state.go

+	switch selectors := value.(type) {
+	case string: //check the string is `inherits` else error
+		if selectors == InheritsYamlValue {
+			hf.Inherits = true


Can be a nit but I'd suggest using:

helmfiles: - path: path/to/subhelmfile.yaml selectorsInherited: true

So that the implementation becomes a bit simpler without the overloading of selectors.

yes, I'll do that

mumoshu · 2019-05-05T04:31:15Z

state/state.go

+	Inherits  bool     //do the sub helmfiles inherits from parent selectors
+}
+
+const InheritsYamlValue = "inherits"


Nit: Can be SelectorsInherits so that it looks a bit more consistent with golang community's convention that uses Where + What for const names

This can be removed completely if we go this way https://github.com/roboll/helmfile/pull/567/files#r281003831

mumoshu · 2019-05-05T04:35:11Z

state/state.go

+					if err := extractSelectorContent(hf, v); err != nil {
+						return err
+					}
+				} else {


Can you by any chance remove this implementation of "dict key as the path to a subhelmfile" in favor of path, so that the implementation becomes simpler, and there's fewer ways to achieve the same goal?

yes, I'll do

mumoshu

Added a few comments mostly about names and clean-ups, but this looks awesome at this point!

And I feel like merging early makes it easier to maintain the feature without less possibility to introduce another code conflict before merging.

Thanks a lot for your great work and the contribution @sgandon!

sgandon changed the title ~~feat(#344): add sub helmfiles explicit selectors~~ WIP - feat(#344): add sub helmfiles explicit selectors Apr 28, 2019

sgandon force-pushed the sgandon/use_sub_helmfile_selectors branch from e142839 to 923f7da Compare April 29, 2019 23:02

sgandon changed the title ~~WIP - feat(#344): add sub helmfiles explicit selectors~~ feat(#344): add sub helmfiles explicit selectors Apr 29, 2019

davidovich reviewed Apr 30, 2019

View reviewed changes

mumoshu reviewed May 2, 2019

View reviewed changes

mumoshu force-pushed the master branch 6 times, most recently from ce5356f to 59b254e Compare May 4, 2019 13:10

sgandon added 4 commits May 4, 2019 22:52

feat(roboll#344): add sub helmfiles explicit selectors

88f1c64

feat(roboll#344): add inherits explicit selectors

184d3dd

feat(roboll#344): change EXPERIMENTAL to HELMFILE_EXPERIMENTAL

7ac33e8

feat(roboll#344): add path: to make sub-helmfiles clearer

8db0021

sgandon force-pushed the sgandon/use_sub_helmfile_selectors branch from 5d54b18 to 8db0021 Compare May 4, 2019 21:07

feat(roboll#344): add another error case and fix test case

deba54e

mumoshu reviewed May 5, 2019

View reviewed changes

mumoshu approved these changes May 5, 2019

View reviewed changes

mumoshu merged commit 4581e00 into roboll:master May 5, 2019

sgandon deleted the sgandon/use_sub_helmfile_selectors branch May 5, 2019 20:40

sgandon mentioned this pull request May 5, 2019

removes dictionary key for subhelm and uses selectorsInherited #576

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(#344): add sub helmfiles explicit selectors #567

feat(#344): add sub helmfiles explicit selectors #567

sgandon commented Apr 27, 2019

sgandon commented Apr 27, 2019

davidovich commented Apr 27, 2019

sgandon commented Apr 28, 2019

sgandon commented Apr 28, 2019

sgandon commented Apr 29, 2019

davidovich Apr 30, 2019

sgandon Apr 30, 2019

mumoshu left a comment

sgandon commented May 2, 2019 •

edited

Loading

sgandon commented May 2, 2019

mumoshu commented May 2, 2019

sgandon commented May 2, 2019

sgandon commented May 4, 2019

sgandon commented May 4, 2019

mumoshu May 5, 2019

sgandon May 5, 2019

mumoshu May 5, 2019

mumoshu May 5, 2019

mumoshu May 5, 2019

sgandon May 5, 2019

mumoshu left a comment

feat(#344): add sub helmfiles explicit selectors #567

feat(#344): add sub helmfiles explicit selectors #567

Conversation

sgandon commented Apr 27, 2019

sgandon commented Apr 27, 2019

davidovich commented Apr 27, 2019

sgandon commented Apr 28, 2019

sgandon commented Apr 28, 2019

sgandon commented Apr 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumoshu left a comment

Choose a reason for hiding this comment

sgandon commented May 2, 2019 • edited Loading

sgandon commented May 2, 2019

mumoshu commented May 2, 2019

sgandon commented May 2, 2019

sgandon commented May 4, 2019

sgandon commented May 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumoshu left a comment

Choose a reason for hiding this comment

sgandon commented May 2, 2019 •

edited

Loading