Allow running helm against a subsets of charts in a file by awwithro · Pull Request #30 · roboll/helmfile

awwithro · 2018-01-25T18:26:31Z

Similar to what's described in #8

Globbing isn't useful for my particular workflow but I do have a large helmfile where its useful to run just one or two charts out of the entire file.

awwithro · 2018-01-25T18:28:15Z


 	"github.com/roboll/helmfile/helmexec"

-	yaml "gopkg.in/yaml.v1"


My IDE runs go fmt so there are a few formatting changes not directly related to the PR. Not sure if you have strong feelings on that.

awwithro · 2018-02-06T17:51:27Z

@roboll ping! Any feedback on this PR?

sstarcher · 2018-02-26T15:48:12Z

Anything needed to get this merged

roboll · 2018-02-26T16:10:15Z

@sstarcher I haven't been using the tool lately, so just time to review and test.

If you (or anyone else) are actively using the tool and can confirm it works as expected, I'll accept the PR.

sstarcher · 2018-02-26T16:16:13Z

I'll make some time to test it out. Thanks

sstarcher · 2018-02-26T16:16:39Z

@roboll are you using something else to accomplish similar functionality to helmfile?

roboll · 2018-02-26T16:30:51Z

@sstarcher nope, tool is still relevant, just not doing anything with helm at the moment.

mumoshu · 2018-02-27T13:49:57Z

I like this feature!

However, I'm afraid if merging this prevents a similar but a more high-level filtering feature like @xeor proposed in #28?
Can I contribute the impl for #28 in addition to this?

sstarcher · 2018-02-27T14:42:49Z

I would not see this as mutually exclusive with #28

awwithro · 2018-02-27T14:47:40Z

I don't see them as mutually exclusive either but personally would use different different helmfiles over tags if the charts were that closely related

…

On Feb 27, 2018 7:43 AM, "Shane Starcher" ***@***.***> wrote: I would not see this as mutually exclusive with #28 <#28> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#30 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AOO2lv87Oj5bmfKLO84x0xfm--5z8UGSks5tZBRqgaJpZM4RtSIh> .

mumoshu · 2018-03-01T13:44:49Z

+	for _, name := range chartNames {
+		missingName := true
+		for _, chart := range state.Charts {
+			if chart.Name == name {


Nearly a nit but I wanted to confirm if, probably you're confusing release name vs chart name?
ChartSpec.Name stands for release name, whereas ChartSpec.Chart is the name of chart used to create the release.
Also see #25 for more context.

I'm going to fix the terminology issue starting #35. So I'd fix it on my side if necessary.

Your intention is to filter releases by release names rather than to filter charts by chart names, right?

Absolutely, I see you've made things more consistent across the board (and more precise) in your PR. I'll rebase and fix my terminology.

awwithro · 2018-03-03T20:24:10Z

@mumoshu PTAL, I've switched the terminology to reference release instead of chart which looks consistent with your rewrite.

mumoshu · 2018-03-04T13:19:34Z

 }

-func (state *HelmState) DiffReleases(helm helmexec.Interface, additonalValues []string) []error {
+func (state *HelmState) DiffReleases(helm helmexec.Interface, additionalValues []string) []error {


Good catch 👍

mumoshu · 2018-03-04T13:40:09Z

 			Usage: "Set namespace. Uses the namespace set in the context by default",
 		},
+		cli.StringSliceFlag{
+			Name:  "release-name, r",


Sorry but honestly saying, I'm not yet sure if this is the right name(if there's one) which describes the feature.
More concretely, I imagine that one would misunderstand this as a flag used for somehow overriding release names, which isn't true of course.

Instead, could this be any of the followings?

helmfile sync --only myrelease1,myrelease2

not very intuitive as it isn't clear that what we can filter & by what we can filter releases

helmfile sync --only-releases myrelease1,myrelease2

it isn't clear that by what we can filter releases

helmfile sync --filter-releases myrelease1,myrelease2

same as 3

helmfile sync --filter-releases-by-names myrelease1,myrelease2

imho, too verbose

helmfile sync --filter-releases name=myrelease1,myrelease2

what if we wanted to support Tags? #28? --filter-releases tag=tier1:frontend? not sure i could correctly differentiate = and : :)

helmfile sync myrelease1,myrelease2

helmfile sync myrelease1 myrelease2

I slightly prefer 7 as it straightly maps to my mental model that helmfile is working against releases in the first place. It would also look familiar to ones used to kubectl. kubectl get po works against all the pods, whereas kubectl get po foo filters the target to a single pod.

A nice side-effect of it is that we now have a natural integration with shell brace expansion e.g. helmfile sync myrelease{1,2} 😃

I would either recommend 7 or helmfile sync --filter name=myrelease Where in the future we could support more filters

helmfile sync --filter name=myrelease seems the most natural to me as well considering the proposed feature to use tags as a filter.

is nice and clean but I don't think would be extended with a tag structure as cleanly. It would be less obvious what the behavior would be with names and tags. helmfile sync --tags tier:frontend myrelease1 myrelease2 would that apply the intersection or union of the two filters? This question stands either way but I think its more clear using a --filter flag that either tags or names should be used.

I suppose 7 could lead to helmfile sync tag:tier=frontend or similar but that doesn't seem as clean to me

@awwithro for supporting and/or something like
helmfile sync --filter name=bob,tier=frontend --filter name=sam
Any release that makes name=bob and tier=frontend AND any chart that has the name sam.

That would allow us to support and/or logic.

that makes sense to me, intersection within a filter and a union of all filters. I'll start work on the implementation

@awwithro thanks that work will help toward the full tag/filter implementation

@awwithro Thanks for your efforts! Please let us know if there's anything we can help. Probably questions regarding implementation, design, testing, etc?

hey @mumoshu I've been a bit busy at work and haven't had a chance to circle back on this quite yet. I did have some questions though. I believe you're on the kubernetes slack? I can find you there

mumoshu · 2018-03-04T13:41:07Z

 	}
 }
+
+func TestReleaseFilter(t *testing.T) {


mumoshu · 2018-03-04T13:43:50Z

@awwithro Wow, thanks for your efforts! The implementation looks awesome. I really appreciate you adding a test, too!

My final point is about how we should name the flag. WDYT?

mumoshu · 2018-03-13T15:06:49Z

yes, lets discuss there! 2018年3月13日(火) 23:51 Alex Withrow <notifications@github.com>:

…

***@***.**** commented on this pull request. ------------------------------ In main.go <#30 (comment)>: > @@ -43,6 +43,10 @@ func main() { Name: "namespace, n", Usage: "Set namespace. Uses the namespace set in the context by default", }, + cli.StringSliceFlag{ + Name: "release-name, r", hey @mumoshu <https://github.com/mumoshu> I've been a bit busy at work and haven't had a chance to circle back on this quite yet. I did have some questions though. I believe you're on the kubernetes slack? I can find you there — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#30 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABV-Wzgcyov1aPhhfZ7wN3UL_Wifveaks5td9zygaJpZM4RtSIh> .

mumoshu · 2018-03-16T07:42:29Z

Started talking with @awwithro towards finishing this

awwithro · 2018-03-21T01:13:01Z

  # Published chart example
  - name: vault                            # name of this release
    namespace: vault                       # target namespace
+    tags:                                  # Arbitrary key value pairs for filtering releases


What do you think about using labels instead of tags? would be consistent with k8s.

Sounds good in general.

One thing I'm wondering on top of that though is - Would it also be reasonable to rename the --tags flag to --selector for more consistency with k8s'?

And in that case, I can't help expecting that labels could be somehow propagated as values to helm charts, so that we can use the same set of labels to filter both releases(helmfile --selector ...) and resulting k8s objects(kubectl get --selector ...). What do you think about that?

I like the -l,--selector standardization. I'll switch to that. As far as propagating labels to the resulting k8s objects. I like the idea. I'm not sure how the implementation would work exactly since the objects would be managed via helm but is out of scope for this PR

awwithro · 2018-03-21T01:15:04Z

@mumoshu PTAL, I refactored things similar to what we discussed with tags/names. What do you think? Anything missing or in need of changing?

sstarcher · 2018-03-21T14:23:39Z

I tested running a single example and was initially confused that the globals don't get passed down to the sub functions

Works
helmfile --tags name=filebeat diff

Does not work
helmfile diff --tags name=filebeat

sstarcher · 2018-03-21T14:25:51Z

Also verified != works

mumoshu · 2018-03-22T06:58:54Z

+- name: myrelease3
+  chart: mychart3
+  tags:
+    tier: backend


Thx for writing an effective test 👍

mumoshu

Just one thing! Otherwise LGTM.

mumoshu · 2018-03-22T07:05:48Z

+			}
+			release.Tags["name"] = release.Name
+			if filter.Match(release) {
+				// Only add a release once


The functionality is awesome!
If I could ask more, probably you can change the nesting so that there would be less code, plus and a bit more efficiency?

var filters := []ReleaseFilter{} for _, tag := range tags { f, err := ParseTags(tag) // snip - error handling filters = append(filters, f) } for _, r := range state.Releases { for _, f := range filters { if f.Match(releases) { filteredReleases = append(filteredReleases, release) continue } } }

mumoshu

Note to self for later follow-up

mumoshu · 2018-03-23T02:18:18Z

  # Published chart example
  - name: vault                            # name of this release
    namespace: vault                       # target namespace
+    tags:                                  # Arbitrary key value pairs for filtering releases


Note: change the remaining tags: to labels:

mumoshu · 2018-03-23T02:18:27Z

+   --file FILE, -f FILE         load config from FILE (default: "helmfile.yaml")
+   --quiet, -q                  silence output
+   --namespace value, -n value  Set namespace. Uses the namespace set in the context by default
+   --tags value                 Only run using the releases that match tags. Tags can take the form of foo=bar or foo!=bar.


Note: change the remaining tags: to labels:

mumoshu · 2018-03-23T02:18:47Z

 - Relative paths referenced on the command line are relative to the current working directory the user is in

 For additional context, take a look at [paths examples](PATHS.md)
+## Tags Overview


Note: Tags -> Labels

sstarcher · 2018-03-23T13:11:53Z

-The `tags` parameter can be specified multiple times. Each parameter is resolved independently so a release that matches any parameter will be used. 
+The `labels` parameter can be specified multiple times. Each parameter is resolved independently so a release that matches any parameter will be used. 
+
+`--selector tier=frontend --selector tier=backend` will select all the charts


this should be --labels right?

--selector,-l is consistent with kubectl and how it selects objects using labels. I'd used -l myself previously but I think having a common operation across tools is desirable.

sstarcher · 2018-03-23T13:13:55Z

+			Name: "selector, l",
+			Usage: `Only run using the releases that match labels. Labels can take the form of foo=bar or foo!=bar.
+	A release must match all labels in a group in order to be used. Multiple groups can be specified at once.
+	--selector tier=frontend,tier!=proxy --selector tier=backend. Will match all frontend, non-proxy releases AND all backend releases.


should all references to selector be changed to labels?

similar to k8s, the idea is that labels are on objects, and a selector selects based on labels

ahh if that's the case the docs that are calling out --label should be changed right?

--labels tier=frontend,tier!=proxy --labels tier=backend. Will match all frontend, non-proxy releases AND all backend releases.

to

--selector tier=frontend,tier!=proxy --selector tier=backend. Will match all frontend, non-proxy releases AND all backend releases.```

Agreed we should use --selector and -l, but we just need to make sure all the docs align.

it should be consistent across the docs now

sstarcher

excellent work thanks for this

sstarcher · 2018-03-23T15:40:52Z

After this is merged could we cut a release for secrets and this.

mumoshu · 2018-03-23T16:06:09Z

@awwithro @sstarcher Thank you very much for your efforts and supports!

awwithro · 2018-03-23T16:18:41Z

thanks @mumoshu @sstarcher for the ideas and feedback!

awwithro commented Jan 25, 2018

View reviewed changes

mumoshu reviewed Mar 1, 2018

View reviewed changes

mumoshu mentioned this pull request Mar 1, 2018

Tags? #28

Closed

awwithro force-pushed the chart_filter branch from 6a13136 to d3cc73a Compare March 3, 2018 19:24

mumoshu reviewed Mar 4, 2018

View reviewed changes

Filter releases on tags

15f826b

awwithro force-pushed the chart_filter branch from f05f06a to 15f826b Compare March 21, 2018 01:11

awwithro commented Mar 21, 2018

View reviewed changes

mumoshu reviewed Mar 22, 2018

View reviewed changes

mumoshu requested changes Mar 22, 2018

View reviewed changes

s/Tags/Labels, cleanup filtering code

1883151

mumoshu reviewed Mar 23, 2018

View reviewed changes

Cleaned up old tag references

4c055db

sstarcher reviewed Mar 23, 2018

View reviewed changes

Alex Withrow added 2 commits March 23, 2018 07:18

more doc cleanup

3256210

consistent on selector and labels now

be260a7

sstarcher approved these changes Mar 23, 2018

View reviewed changes

mumoshu merged commit 4b08ea9 into roboll:master Mar 23, 2018


		"github.com/roboll/helmfile/helmexec"

		yaml "gopkg.in/yaml.v1"

Conversation

awwithro commented Jan 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awwithro commented Feb 6, 2018

Uh oh!

sstarcher commented Feb 26, 2018

Uh oh!

roboll commented Feb 26, 2018

Uh oh!

sstarcher commented Feb 26, 2018

Uh oh!

sstarcher commented Feb 26, 2018

Uh oh!

roboll commented Feb 26, 2018

Uh oh!

mumoshu commented Feb 27, 2018

Uh oh!

sstarcher commented Feb 27, 2018

Uh oh!

awwithro commented Feb 27, 2018 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awwithro commented Mar 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awwithro Mar 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mumoshu commented Mar 4, 2018

Uh oh!

mumoshu commented Mar 13, 2018 via email

Uh oh!

mumoshu commented Mar 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awwithro commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sstarcher commented Mar 21, 2018

Uh oh!

sstarcher commented Mar 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mumoshu left a comment

Choose a reason for hiding this comment

Uh oh!

mumoshu Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

awwithro Mar 4, 2018 •

edited

Loading

awwithro commented Mar 21, 2018 •

edited

Loading

mumoshu Mar 22, 2018 •

edited

Loading