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

fea(#507): support assign --post-renderer flags , helmDefaults config , release config when use helm v3 #510

Merged
merged 16 commits into from
Dec 13, 2022

Conversation

tanguofu
Copy link

@tanguofu tanguofu commented Nov 10, 2022

Signed-off-by: guofutan guofutan@tencent.com

This implement reuse the args in helmfile args and helmDefaults config of helmfile.yaml, these PR to maske sure the less change to helmfile code, but make sure the forward compatbility of the helmfile.yaml syntx

@yxxhero yxxhero changed the title fea(#507): support assign --post-renderer and `--post-renderer-arg… fea(#507): support assign --post-renderer and --post-renderer-args within args in helmDefaults when use helm v3 Nov 10, 2022
// reset the postRenderers and filter the post-renderer args from args and put into helm.postRenderers
helm.postRenderers = nil
for _, arg := range args {
if strings.HasPrefix(arg, "--post-renderer=") || strings.HasPrefix(arg, "--post-renderer-args=") {
Copy link
Member

Choose a reason for hiding this comment

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

when use a format like the below:

--post-renderer  value

what will happen?

Copy link
Author

@tanguofu tanguofu Nov 11, 2022

Choose a reason for hiding this comment

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

this format will be deal with analyzeArgs , and keep the args order then past into helm command:
such as :

helmfile -e ksp -f ti-k8s/helmfile.yaml -l chart=mysql  --args="--post-renderer rewrite-ksp.sh"  --debug

the helm command is :

 helm template mysql mysql --namespace ti-inf --values /tmp/helmfile3923818193/ti-inf-mysql-values-674855cccb --values /tmp/helmfile1141692238/ti-inf-mysql-values-9b4dd874f --values /tmp/helmfile2042720088/ti-inf-mysql-values-5448c6fcbd --values /tmp/helmfile1303814841/ti-inf-mysql-values-776bd65866 --post-renderer rewrite-ksp.sh --debug

so these will also work well.

Copy link
Member

Choose a reason for hiding this comment

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

@tanguofu

if strings.HasPrefix(arg, "--post-renderer=") || strings.HasPrefix(arg, "--post-renderer-args=") {

will --post-renderer=vaule --post-renderer value all be handled?

@yxxhero
Copy link
Member

yxxhero commented Nov 10, 2022

@tanguofu Need most units and e2e tests.

@yxxhero
Copy link
Member

yxxhero commented Nov 11, 2022

@tanguofu BTW. please fix ci issue and add tests.

Copy link
Contributor

@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.

Hey! I just took a glance at your awesome work. One question- Although you mentioned helmDefaults in the PR title, the actual change does not contain anything related to helmDefaults.
Are you also willing to add the post renderer support to helmDefaults too? That would be great if so.

@tanguofu
Copy link
Author

@tanguofu BTW. please fix ci issue and add tests.

thanks to @yxxhero for careful CR!

i add the process when the args like --post-renderer=”xxx xx.sh“
and add unit test at func Test_SetExtraArgs(t *testing.T) fix the golangci-lint run errors .
may u take a review again ?
thanks very much.

@tanguofu
Copy link
Author

tanguofu commented Nov 11, 2022

Hey! I just took a glance at your awesome work. One question- Although you mentioned helmDefaults in the PR title, the actual change does not contain anything related to helmDefaults. Are you also willing to add the post renderer support to helmDefaults too? That would be great if so.

i think it is not a good idea to add a new config post-renderer inito helmDefaults after me write the PR. instead i think it is better to reuse args config in helmDefaults; becasue:

  1. add a new config is harmful for previous versions helmfile.yaml (ie maybe some tools which parse helmfile.yaml the suppose that is invariable)
  2. in code implement there will be more files and codes change to helmfile codebase. which also take more cost to test。

and more resue the args also make these command works :

helmfile -e ksp -f ti-k8s/helmfile.yaml -l chart=mysql  --args="--post-renderer rewrite-ksp.sh"

this will make this feature more easy to use.

@mumoshu
Copy link
Contributor

mumoshu commented Nov 12, 2022

@tanguofu Helmfile is designed for use when you want to manage a set of releases declaratively. If you need a specific post-renderer to make your release(s) get deployed properly, I consider the post-renderer a part of the deployment, which means it should be covered in helmfile.yaml.

add a new config is harmful for previous versions helmfile.yaml (ie maybe some tools which parse helmfile.yaml the suppose that is invariable)

We don't retain forward compatibility in Helmfile. I think that's generally the same stance as other projects within the K8s community. We do maintain backward compatibility. In the context of backward compatibility, adding new fields is fine.

@tanguofu
Copy link
Author

@tanguofu Helmfile is designed for use when you want to manage a set of releases declaratively. If you need a specific post-renderer to make your release(s) get deployed properly, I consider the post-renderer a part of the deployment, which means it should be covered in helmfile.yaml.

add a new config is harmful for previous versions helmfile.yaml (ie maybe some tools which parse helmfile.yaml the suppose that is invariable)

We don't retain forward compatibility in Helmfile. I think that's generally the same stance as other projects within the K8s community. We do maintain backward compatibility. In the context of backward compatibility, adding new fields is fine.

well let me add the post-renderer configs in helmDefaults of helmfile.yaml.

@tanguofu
Copy link
Author

tanguofu commented Nov 14, 2022

the postRenderer and postRendererArgs were both add in helmDafult of helmfile.yaml, please @mumoshu review again

many thanks !

Comment on lines 115 to 121
if len(state.HelmDefaults.PostRenderer) > 0 {
argArr = append(argArr, fmt.Sprintf("--post-renderer=\"%s\"", state.HelmDefaults.PostRenderer))
}
for _, arg := range state.HelmDefaults.PostRendererArgs {
argArr = append(argArr, fmt.Sprintf("--post-renderer-args=\"%s\"", arg))
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is not the right place to add this code- The helmDefaults args, the under pinning of the --args flag of helmfile, has known to be not well-designed, and being removed in Helmfile v1. The main difficulty is that Helmfile has no way to know which args specified in the helmDefaults.args to be passed to which helm sub-commands (install, uninstall, list, diff, etc) a helmfile command is going to run.

More importantly, the post renderer should be passed down to chartify when you're going to use a directory of raw k8s manifests as a chart, or a kustomize config as a chart, or you're using some chartify-provided features like ad-hoc dependencies and ad-hoc patching(jsonPatches and stragicMergePatches, forceNamespace, and so on).

Still curious- did this actually work on your machine? 🤔

Copy link
Author

@tanguofu tanguofu Nov 14, 2022

Choose a reason for hiding this comment

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

well, the --args flag of helmfile is easy to use indeed, such as:

helmfile -e ksp -f ti-k8s/helmfile.yaml -l chart=mysql  --args="--post-renderer=`pwd`rewrite-ksp.sh"

as the post renderer accept a abs path of exec script, this works well in our project;

though it is more chartify to special each flags to parse the flags to helm exactly; but based on what I know, it is need to refactor lots of code, may be change the helmexec.Interface and add some set methods
? and invoke at every place of helmexec.SetExtraArgs ?

So what is the good way to implement this feature? @mumoshu may you give some guidelines please ?

thanks very much

Copy link
Contributor

@mumoshu mumoshu Nov 15, 2022

Choose a reason for hiding this comment

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

this works well in our project;

Yep. But it's not guaranteed to work. We don't have a complete design doc and tests to make sure your usage of "--args" works. My understanding was that Helmfile must have been implemented to pass the args to every helm sub-commands, regardless of if the sub-command supports the set of args or not.

Also note that it's being removed in Helmfile v1 https://github.com/helmfile/helmfile/blob/main/docs/proposals/towards-1.0.md

Copy link
Contributor

@mumoshu mumoshu Nov 15, 2022

Choose a reason for hiding this comment

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

the good way to implement this feature

I'd recommend following the same pattern that is used for other Helmfile flags and fields. Propagate Helmfile flags to helmDefaults or Override<WHATEVER> fields, inherit helmDefaults to releases[] fields if necessary, apply overrides from flags if necessary, and finally compose chartify parameters and helm flags from the final releases[] entry fields.

Choose a reason for hiding this comment

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

@tanguofu Can you implement the change suggested by @mumoshu? The solution with the possibility to set the post-rendering through flags, helmDefaults or per release sounds perfect. Missing Post-rendering is the only thing stopping me from switching to Helmfile, so I'd be very grateful. Keep up the good work, guys!

Copy link
Author

Choose a reason for hiding this comment

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

ok, i will implement the change, please just wait a moment.

@yxxhero
Copy link
Member

yxxhero commented Dec 4, 2022

@tanguofu any updates?

@sherifkayad
Copy link

@mumoshu any updates on this one?

@KR411-prog
Copy link

This is very useful feature. Will this be merged soon?

@yxxhero
Copy link
Member

yxxhero commented Dec 11, 2022

@tanguofu waiting for your updates. thanks very much.

@mumoshu
Copy link
Contributor

mumoshu commented Dec 11, 2022

@sherifkayad @KR411-prog This is an open-source project driven by volunteers. Instead of a storm of "any updates / eta?" messages that force anyone to work on it for you for free, you and we should all be encouraged to contribute via code, test results, or sponsorships that help the work to be done, if you need it!

@tanguofu
Copy link
Author

@tanguofu waiting for your updates. thanks very much.

sorry for late to update here,it is a bit lot work to fix by @mumoshu guides。
So this patch only implement --post-renderer flags first, because this make change less and more easy to keep the specification of the helmfile. and this is enough as user can wrapper post-renderer with a bash script.

If this patch canbe merge i'd like to implement --post-renderer-args later.

may i have @mumoshu to take code review again, As i am unpracticed to the strict code sytle,many thanks @mumoshu @yxxhero to give me advice or comments。

@sherifkayad
Copy link

sherifkayad commented Dec 12, 2022

@mumoshu That's an interesting take on questions / issues placed to your projects .. in years of using / trying (as much as I can) to enhance projects that I find doing something useful, that's really the first time I get such an unprofessional response from a maintainer .. anyways let's just get a few things clear here:

  • I do apologize if my question was interpretted as "pressure" on people to do stuff ..
  • When you say "me benfiting" or "me using" or whatever personal points, I do believe you are wrong! .. Many open source users will benefit from that not only "me"!
  • Someone already contributed a PR that was up for review and I just wanted to see if it got in the ice box or if it was still worked on / prioritized
  • It's absolutely your right as the maintainer to simply say: "sorry folks this is not a priority for us" .. instead of just storming out at people for asking about something they wanted!
  • On why not simply contributing, well, you have to understand that not everyone that's using stuff, can really contribute! .. One dumb simple reason is e.g. know-how .. I for instance am too far away from being able to write Golang in a way that will satisfy even the least standards .. so basically I wasn't just lazy contributing and busy making money out of your open source project!

@yxxhero
Copy link
Member

yxxhero commented Dec 12, 2022

@tanguofu Thanks very much.

@mumoshu
Copy link
Contributor

mumoshu commented Dec 12, 2022

@sherifkayad From a professional volunteer opensource maintainer who usually works in mornings, nights and Sundays for everyone in the world for almost free and kept it for years, and who gets asked "What's ETA of something" and "Is there any update on this" thing so often, it's my responsibility to clearly state my stance, to avoid us maintainers and opensource contributors from burning out. I take that's one of my jobs as the primary maintainer of this project. If I didn't do it, who does it? Who saves us from burning out?

Also, you can always say something like "cheers!" or prefix your message with "Thanks a lot for all your efforts here! But ..." or something like that. Isn't that what we usually do in our real lives..? At least that's how I treat the maintainer or the contributor outside of this project, where I'm just an user of an opensource project.

All that said,

It's absolutely your right as the maintainer to simply say: "sorry folks this is not a priority for us" .. instead of just storming out at people for asking about something they wanted!

I have something to learn from this 😉 Thank you for your feedback. But again, at the same time, I hope every open-source user is more thoughtful about who's volunteering unpaid hours to respond to all your never-ending requests. That's the right behavior if you're maintaining an open-source project that is a part of your open-core business. However, Helmfile is different- Helmfile isn't tied to any business entity and it's completely driven by volunteers, no VC backed.

Sorry if this all sounded very "unprofessional". But that's how I continued this "job" for years.

Lastly, I tend to just put a 👍 on the pull request when I absolutely need something to be merged in a random repo, so that you can express that you need it without inviting more pressuring messages to the pull request. Just my two cents.

@mumoshu
Copy link
Contributor

mumoshu commented Dec 12, 2022

@sherifkayad Don't get me wrong. I do this to everyone who does the same. I'm not personally blaming you (sorry if it sounded so). It's not your fault at all if you have no time to contribute code or anything. It's due to the broken open-source economy. But still, I have to protect folks from burning out today, and, well, what you call "storming" turned out to be the only way that worked for me, unfortunately.

@mumoshu
Copy link
Contributor

mumoshu commented Dec 12, 2022

Well sorry for going too out-of-topic.

@tanguofu Thanks for the update! I've just reviewed your changes and almost everything looks great! One thing I have noticed is that you might need to add the post-renderer support also to our helm-diff wrapper code that is invoked on helmfile-diff and helmfile-apply. Let me see if I can add the support myself.

UPDATE: 6c861ba should do the job.

So far I was able to manually test it to work with:

post-renderer.sh

#!/usr/bin/env bash

cat $1
echo "---"

cat <<EOS
apiVersion: v1
kind: ConfigMap
data:
  bar: BAR
metadata:
  name: bar
EOS

helmfile.postrenderer.yaml:

releases:
- name: myapp
  chart: incubator/raw
  values:
  - resources:
    - apiVersion: v1
      kind: ConfigMap
      metadata:
        name: foo
      data:
        foo: FOO
  postRenderer: "./post-renderer.sh"

Run helmfile -f helmfile.postrenderer.yaml diff and see the diff contains two releases, one for the configmap defined in values, another for the configmap injected by the postrenderer script.

$ ./helmfile -f helmfile.postrenderer.yaml diff 
Comparing release=myapp, chart=incubator/raw
********************

        Release was not present in Helm.  Diff will show entire contents as new.

********************
default, bar, ConfigMap (v1) has been added:
- 
+ apiVersion: v1
+ kind: ConfigMap
+ data:
+   bar: BAR
+ metadata:
+   name: bar
default, foo, ConfigMap (v1) has been added:
- 
+ # Source: raw/templates/resources.yaml
+ apiVersion: v1
+ data:
+   foo: FOO
+ kind: ConfigMap
+ metadata:
+   labels:
+     app: raw
+     chart: raw-0.2.5
+     heritage: Helm
+     release: myapp
+   name: foo

@yxxhero
Copy link
Member

yxxhero commented Dec 12, 2022

@mumoshu I support you.

@mumoshu
Copy link
Contributor

mumoshu commented Dec 12, 2022

Let me try if I can distill my manual test procedure into a new E2E test-case to keep it working. Once that's clear, I think this is ready to be merged.
Do you have anything to add other than that? @yxxhero

@yxxhero
Copy link
Member

yxxhero commented Dec 12, 2022

@mumoshu I think all is good.

@yxxhero
Copy link
Member

yxxhero commented Dec 12, 2022

@tanguofu maybe need some docs.

@mumoshu
Copy link
Contributor

mumoshu commented Dec 12, 2022

Added an E2E test-case via d5bd31d

@yxxhero
Copy link
Member

yxxhero commented Dec 13, 2022

@mumoshu seem like we should add helm-diff when e2e.

@yxxhero yxxhero changed the title fea(#507): support assign --post-renderer and --post-renderer-args within args in helmDefaults when use helm v3 fea(#507): support assign --post-renderer within args in helmDefaults when use helm v3 Dec 13, 2022
@tanguofu tanguofu changed the title fea(#507): support assign --post-renderer within args in helmDefaults when use helm v3 fea(#507): support assign --post-renderer flags , helmDefaults config , release config when use helm v3 Dec 13, 2022
@yxxhero
Copy link
Member

yxxhero commented Dec 13, 2022

@tanguofu I will help you to fix e2e test.

guofutan and others added 16 commits December 13, 2022 13:12
…` within args in helmDefaults when use helm v3

Signed-off-by: guofutan <guofutan@tencent.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
…patibility when there is blank in the args values.

Signed-off-by: guofutan <guofutan@tencent.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
…fault of helmfile.yaml

Signed-off-by: guofutan <guofutan@tencent.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
…elmdefault or release config

1. only implement post-renderer flags this patch
2. As mumoshu advise, add helmfile flags `--post-render` and add the
   postRenderer  config in helmDefaults and release. the priority is
   helmfile flags > release > helmDefaults.
3. fix the test case in state_test.go and some other tests.

Signed-off-by: guofutan <guofutan@tencent.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
Signed-off-by: Quan TRAN <account@itscaro.me>
Signed-off-by: yxxhero <aiopsclub@163.com>
* Allow running images with users other than root

Signed-off-by: Patrick Hobusch <patrick@hobusch.net>
Signed-off-by: yxxhero <aiopsclub@163.com>
Signed-off-by: yxxhero <aiopsclub@163.com>

Signed-off-by: yxxhero <aiopsclub@163.com>
Co-authored-by: Quan TRAN <itscaro@users.noreply.github.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
All the dependencies get correctly installed when dealing with remote
charts.

If there's a local chart that depends on remote dependencies then those
don't get automatically installed. See #526. They end up with this
error:

```
Error: no cached repository for helm-manager-b6cf96b91af4f01317d185adfbe32610179e5246214be9646a52cb0b86032272 found. (try 'helm repo update'): open /root/.cache/helm/repository/helm-manager-b6cf96b91af4f01317d185adfbe32610179e5246214be9646a52cb0b86032272-index.yaml: no such file or directory
```

One workaround for that would be to add the repositories from the local
charts. Something like this:

```
cd local-chart/ && helm dependency list $dir 2> /dev/null | tail +2 | head -n -1 | awk '{ print "helm repo add " $1 " " $3 }' | while read cmd; do $cmd; done
```

This however is not trivial to parse and implement.

An easier fix which I did here is just to not allow doing
`--skip-refresh` for local repositories.

Fixes #526

Signed-off-by: Indrek Juhkam <indrek@urgas.eu>

Signed-off-by: Indrek Juhkam <indrek@urgas.eu>
Signed-off-by: yxxhero <aiopsclub@163.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
@yxxhero
Copy link
Member

yxxhero commented Dec 13, 2022

@tanguofu @mumoshu maybe it require a k8s cluster to pass the e2e test.

@mumoshu
Copy link
Contributor

mumoshu commented Dec 13, 2022

@yxxhero Ah true! I didn't realize it until now.. Shall I revert a3e8c04 so that we can merge this anyway?

@mumoshu mumoshu merged commit 89d9a9b into helmfile:main Dec 13, 2022
@mumoshu
Copy link
Contributor

mumoshu commented Dec 13, 2022

Oops I pushed something to main by mistake and that closed this pull request... Let me fix it and resubmit this.

@mumoshu
Copy link
Contributor

mumoshu commented Dec 13, 2022

Okay we already disabled the ability to force-push main. Let's go ahead. Forget about the dirty commits and make the necessary changes in follow-up PRs 🙏

@mumoshu
Copy link
Contributor

mumoshu commented Dec 13, 2022

@tanguofu Thanks again for your contribution! We'd appreciate it if you could help in writing documentation about this feature when you have some time.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
9 participants