feat: show live output from the Helm binary #286

rodrigorfk · 2022-08-11T10:49:12Z

This PR addresses what has been discussed here: #283

To enable some workflows where is interesting to have the stdout/stderr output of the Helm binary being shown live in the Helmfile output, the --enable-live-output global flag can now be provided to switch the default behaviour from Helmfile, which is to show the output from the Helm binary only after the command has completed.

mumoshu · 2022-08-12T01:50:54Z

pkg/state/state.go

@@ -42,6 +42,7 @@ const (
 // ReleaseSetSpec is release set spec
 type ReleaseSetSpec struct {
 	DefaultHelmBinary string `yaml:"helmBinary,omitempty"`
+	EnableLiveOutput  *bool  `yaml:"enableLiveOutput,omitempty"`


I believe this is more like an operational feature than a part of helm releases declared in helmfile.yaml. helmfile.yaml is meant to contain anything for helm releases that are to be templated/diffed/applied/linted by helmfile, but how commands like helm/diff/apply/lint/etc are run.
That said, can we remove this from ReleaseSetSpec and provide it as a command-line flag only, so that it's more consistent with other operational flags?

Hi @mumoshu , thanks for the feedback and for taking the time to explain the concept around operational flags, completely understood and definitely make sense, I will address your suggested change shortly and will circle back here once done.

mumoshu · 2022-08-14T02:57:41Z

pkg/helmexec/exec.go

@@ -484,7 +490,7 @@ func (helm *execer) exec(args []string, env map[string]string) ([]byte, error) {
 	}
 	cmd := fmt.Sprintf("exec: %s %s", helm.helmBinary, strings.Join(cmdargs, " "))
 	helm.logger.Debug(cmd)
-	outBytes, err := helm.runner.Execute(helm.helmBinary, cmdargs, env)
+	outBytes, err := helm.runner.Execute(helm.helmBinary, cmdargs, env, helm.enableLiveOutput)


I believe there are some execer methods that needs to return the whole output to be consumed by the caller, like execer.List.

For example, the List method returns the whole helm-list output so that the caller can use it to decide if a helm release is already installed onto the cluster or not, which affects what helmfile sync and helmfile apply do when the release exists in the cluster and is requested to be uninstalled by setting installed: false in helmfile.yaml like:

releases: - name: myapp # This instructs Helmfile to uninstall the release by running `helm uninstall myapp` if `helm list` output showed the `myapp` release as installed installed: false

Perhaps we'd need to make the enableLiveOutput flag a parameter to the execer.exec function, so that each caller execer function can decide which value to be provided to exec, either helm.enableLiveOutput or a hard-coded false. WDYT? 🤔

thanks again for the heads up @mumoshu, I have followed your recommendation and added the ability of the caller to override the enableLiveOutput when calling execer.exec by adding a new pointer parameter, callers that want to explicitly disable live output will pass it as a reference for a hard-coded false, other callers that don't care about it, will pass it as a nil, and then helm.enableLiveOutput will be used by default inside the execer.exec function

Hi @mumoshu, sorry for the direct mention here, but I just want to check your opinion about the final implementation in this PR, do you believe is it good enough? do you suggest any change or further tests? thanks in advance.

yxxhero · 2022-08-14T07:32:20Z

@rodrigorfk Thanks very much. Please fix the conflicts.

yxxhero · 2022-08-24T11:13:48Z

@rodrigorfk Please fix the conflic. Thanks very much.

rodrigorfk · 2022-08-25T08:45:44Z

done @yxxhero , conflicts resolved. Thanks for notifying me.

pkg/helmexec/runner.go

Signed-off-by: Rodrigo Fior Kuntzer <rodrigo@miro.com>

mumoshu · 2022-09-17T11:01:58Z

Sorry for the delayed review/merge and for requiring you to resolve conflicts so many times. There're another conflicts now but let me do the resolution this time @rodrigorfk

.github/workflows/ci.yaml

yxxhero · 2022-09-17T13:20:44Z

@mumoshu we only test helm 3.9 in ci? I think if we test tree minor version will be better? 3.7.x 3.8.x 3.9.x?

mumoshu · 2022-09-17T14:40:08Z

@yxxhero See https://helm.sh/docs/topics/version_skew/#supported-versions- this reads like helm maintainers don't maintain older minor releases, right?

yxxhero · 2022-09-17T14:48:40Z

maybe:

Applicable fixes, including security fixes, are cherry-picked into the release branch, depending on severity and feasibility.

In addition, I think the version release strategy of helm should not conflict with the version of HELM we want to support. Generally speaking, we need to support three minor versions. I remember this is the default behavior of open source projects. WDYT? @mumoshu

yxxhero · 2022-09-17T14:56:06Z

As of Helm 3, Helm is assumed to be compatible with n-3 versions of Kubernetes it was compiled against.

@mumoshu

mumoshu · 2022-09-17T15:23:08Z

we need to support three minor versions

This is where I'm lost- what's the rationale behind it if we need to maintain support for unmaintained helm minor versions?

yxxhero · 2022-09-18T00:14:27Z

@mumoshu

Test versions are reduced from 6 to 3, and ci time is greatly reduced
There are many users of the old version of helm. We need to consider that most users will not directly upgrade to the latest version of helm
Only the helm latest version is tested in ci, which maybe will cause the illusion that helmfile only supports the latest version

Thanks very much.

mumoshu · 2022-09-18T00:20:53Z

@yxxhero The last question from my side- doesn't that result in giving our users an illusion that you can "safely" use older helm minor versions? In reality, old helm releases don't even seem to get security patches. Then, regardless of Helmfile supports older helm minor releases or not, you must upgrade Helm to the latest ASAP. Helmfile should not advocate uses of older Helm minor releases. Instead, it should guide users to upgrade Helm earlier.

What if we maintain compatibility with not 3 but 2 helm versions, the latest maintained releases, and the latest unmaintained release? Would that be enough to cover the time needed to upgrade Helm?

yxxhero · 2022-09-18T00:31:56Z

@mumoshu What you said is reasonable. It's really a problem In this way, I think it's better to do as you say.

pkg/config/global.go

yxxhero · 2022-09-18T02:41:13Z

@mumoshu WDYT? about this PR.

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

mumoshu · 2022-09-18T04:44:19Z

@yxxhero Thanks for your confirmation! I've fixed the ci workflow accordingly through 47a9838.

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

yxxhero · 2022-09-18T04:47:14Z

@mumoshu ok. you can assign it to me. you need more to rest.

mumoshu · 2022-09-18T05:06:01Z

@yxxhero Thanks for your kind words! I believe this one is ready to be merged. But it would be definitely great if you could handle other issues/prs!

mumoshu

LGTM. Thanks a lot for your patience and contribution @rodrigorfk!

rodrigorfk · 2022-09-19T10:42:47Z

thank you @mumoshu for all the support and suggestions during the review process here, it was an amazing experience being part of it and seeing how a constructive and transparent discussion took place, well done you all.

as a final question from my side: how frequently do you ship a new release covering the last added features? I saw the latest version was released 5 days ago, is there any new release planned? thanks in advance.

mumoshu · 2022-09-19T23:37:45Z

@rodrigorfk Thank you so much for your contribution and kind words! This is absolutely a kind of collaboration that motivates us, maintainers, ❤️

I've just released v0.146.0 for this and other enhancements made since the last patch release. It would be awesome if you could give it a shot and leave any comments/feedback if anything added in the release doesn't work for you!

https://github.com/helmfile/helmfile/releases/tag/v0.146.0

rodrigorfk requested review from mumoshu, itscaro and yxxhero as code owners August 11, 2022 10:49

rodrigorfk force-pushed the enable-live-output branch from 23117c9 to 22e06cc Compare August 11, 2022 11:18

mumoshu reviewed Aug 12, 2022

View reviewed changes

rodrigorfk force-pushed the enable-live-output branch from 22e06cc to b072c56 Compare August 12, 2022 12:49

mumoshu reviewed Aug 14, 2022

View reviewed changes

rodrigorfk force-pushed the enable-live-output branch from b072c56 to 30b7f7b Compare August 16, 2022 15:27

rodrigorfk requested a review from mumoshu August 16, 2022 15:31

mumoshu force-pushed the enable-live-output branch from 30b7f7b to 2fa3e6e Compare August 18, 2022 11:30

rodrigorfk force-pushed the enable-live-output branch from 2fa3e6e to 9fc4829 Compare August 20, 2022 07:59

rodrigorfk force-pushed the enable-live-output branch from 9fc4829 to 0aa6a9c Compare August 25, 2022 08:44

rodrigorfk force-pushed the enable-live-output branch 4 times, most recently from 29b3a5a to ab3ec42 Compare August 29, 2022 10:55

itscaro reviewed Aug 31, 2022

View reviewed changes

pkg/helmexec/runner.go Show resolved Hide resolved

yxxhero force-pushed the enable-live-output branch from ab3ec42 to 6d0adfe Compare September 1, 2022 09:03

rodrigorfk force-pushed the enable-live-output branch from 6d0adfe to d8f7ce2 Compare September 6, 2022 13:20

rodrigorfk requested review from itscaro and mumoshu and removed request for mumoshu and itscaro September 7, 2022 13:57

feat: show live output from the Helm binary

57e6cd8

Signed-off-by: Rodrigo Fior Kuntzer <rodrigo@miro.com>

rodrigorfk force-pushed the enable-live-output branch from d8f7ce2 to 57e6cd8 Compare September 12, 2022 18:45

mumoshu reviewed Sep 17, 2022

View reviewed changes

.github/workflows/ci.yaml Show resolved Hide resolved

yxxhero reviewed Sep 18, 2022

View reviewed changes

pkg/config/global.go Outdated Show resolved Hide resolved

Merge branch 'main' into enable-live-output

47a9838

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

mumoshu force-pushed the enable-live-output branch from 6adebfd to 47a9838 Compare September 18, 2022 04:43

fixup! Merge branch 'main' into enable-live-output

9e449f6

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

mumoshu approved these changes Sep 18, 2022

View reviewed changes

mumoshu merged commit 8408b02 into helmfile:main Sep 18, 2022

mumoshu mentioned this pull request Sep 18, 2022

fix(oci): clean dead code #290

Merged

github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: show live output from the Helm binary #286

feat: show live output from the Helm binary #286

rodrigorfk commented Aug 11, 2022 •

edited

mumoshu Aug 12, 2022

rodrigorfk Aug 12, 2022

mumoshu Aug 14, 2022 •

edited

rodrigorfk Aug 16, 2022

rodrigorfk Aug 19, 2022

yxxhero commented Aug 14, 2022 •

edited

yxxhero commented Aug 24, 2022

rodrigorfk commented Aug 25, 2022

mumoshu commented Sep 17, 2022

yxxhero commented Sep 17, 2022

mumoshu commented Sep 17, 2022

yxxhero commented Sep 17, 2022

yxxhero commented Sep 17, 2022 •

edited

mumoshu commented Sep 17, 2022 •

edited

yxxhero commented Sep 18, 2022

mumoshu commented Sep 18, 2022 •

edited

yxxhero commented Sep 18, 2022

yxxhero commented Sep 18, 2022

mumoshu commented Sep 18, 2022 •

edited

yxxhero commented Sep 18, 2022 •

edited

mumoshu commented Sep 18, 2022

mumoshu left a comment

rodrigorfk commented Sep 19, 2022

mumoshu commented Sep 19, 2022

feat: show live output from the Helm binary #286

feat: show live output from the Helm binary #286

Conversation

rodrigorfk commented Aug 11, 2022 • edited

mumoshu Aug 12, 2022

Choose a reason for hiding this comment

rodrigorfk Aug 12, 2022

Choose a reason for hiding this comment

mumoshu Aug 14, 2022 • edited

Choose a reason for hiding this comment

rodrigorfk Aug 16, 2022

Choose a reason for hiding this comment

rodrigorfk Aug 19, 2022

Choose a reason for hiding this comment

yxxhero commented Aug 14, 2022 • edited

yxxhero commented Aug 24, 2022

rodrigorfk commented Aug 25, 2022

mumoshu commented Sep 17, 2022

yxxhero commented Sep 17, 2022

mumoshu commented Sep 17, 2022

yxxhero commented Sep 17, 2022

yxxhero commented Sep 17, 2022 • edited

mumoshu commented Sep 17, 2022 • edited

yxxhero commented Sep 18, 2022

mumoshu commented Sep 18, 2022 • edited

yxxhero commented Sep 18, 2022

yxxhero commented Sep 18, 2022

mumoshu commented Sep 18, 2022 • edited

yxxhero commented Sep 18, 2022 • edited

mumoshu commented Sep 18, 2022

mumoshu left a comment

Choose a reason for hiding this comment

rodrigorfk commented Sep 19, 2022

mumoshu commented Sep 19, 2022

rodrigorfk commented Aug 11, 2022 •

edited

mumoshu Aug 14, 2022 •

edited

yxxhero commented Aug 14, 2022 •

edited

yxxhero commented Sep 17, 2022 •

edited

mumoshu commented Sep 17, 2022 •

edited

mumoshu commented Sep 18, 2022 •

edited

mumoshu commented Sep 18, 2022 •

edited

yxxhero commented Sep 18, 2022 •

edited