Various U/X improvements for helmfile apply
#586
Conversation
This improves the U/X of `helmfile apply`, by allowing you to selectively apply sub-helmfiles. When you have two or more sub-helmfiles processed, typing `n` to cancel the first doesn't automatically stop the whole helmfile execution. Instead, it proceeds by diffing the next sub-helmfile, and asks you to apply it, which should be what the user would expect. To support this workflow, I have suppressed useless exec logs, correct exit status when diff exists in sub-helmfiles but not in the parent helmfile, and made the final error message emitted by helmfile better. More concretely, this moves more output from `helm` to STDERR and the `debug` log-level. The overall output from `helmfile` should be a bit more cleaner especially for `apply`, `sync`, `diff` and perhaps other `helmfile` sub-commands, too. For example, when one of release failed, `helmfile`'s final error message now includes the error message from the failed `helm` execution, like seen in the last line: ``` List of updated releases : RELEASE CHART VERSION envoy stable/envoy 1.5.0 List of releases in error : RELEASE envoy2 in ./helmfile.yaml: in .helmfiles[0]: in /Users/c-ykuoka/helmfile/helmfile.1.yaml: failed processing release envoy2: helm exited with status 1: Error: UPGRADE FAILED: "envoy2" has no deployed releases ``` This way you can better understand what caused helmfile to finally fail. `helmfile` has been streaminig a lot of stdout and stderr contents from the `helm` commands regardless of the helmfile's log-level. It has been suppressed by default and moved to the `debug` log-level. You will see that it helps you focus on what was the cause of a failure. While working on the above, I found an another bug that made `--detailed-exitcode` useless in some case. That is, `helmfile diff --detailed-exitcode`, when any diff existed only in sub-helmfiles, has been returning an exit code of `1`. It should return `2` when any release had diff and no release had an error, regardless of the target is a sub-helmfile or a parent helmfile. Why? Because that's what `--detailed-exitcode` meant for! After this PR gets merged, `helmfile diff --detailed-exitcode` propery return exit code `2` in such cases. Fixes #543 Resolves #540
|
fanstastic proposal, I love the error message detail ! |
state/state.go
Outdated
| valfile, err := filepath.Abs(value) | ||
| if err != nil { | ||
| errs = append(errs, &ReleaseError{release, err}) | ||
| errs = append(errs, &ReleaseError{release, err, 1}) |
There was a problem hiding this comment.
that would be great if 1 would be a constant with a name so that we know what is this parameter, it would make the code easier to read.
There was a problem hiding this comment.
Good point! I'd introduce three constants, ExitStatusSucess, ExitStatusFailure, ExitStatusDiff for 0, 1, 2 respectively.
There was a problem hiding this comment.
I ended up just avoid using magic numbers whereas possible. Hope it makes sense now!
|
I am sorry I don't have much time to do a full review (I am on vacation tonight and need to finish some things) but that looks ok to me. |
|
Apologies for bloating this PR with my comment, but: Assuming you have a list of charts to apply in a |
|
@aegershman Hey!
|
This improves the U/X of
helmfile apply, by allowing you to selectively apply sub-helmfiles.When you have two or more sub-helmfiles processed, typing
nto cancel the first doesn't automatically stop the whole helmfile execution.Instead, it proceeds by diffing the next sub-helmfile, and asks you to apply it, which should be what the user would expect.
To support this workflow, I have suppressed useless exec logs, correct exit status when diff exists in sub-helmfiles but not in the parent helmfile, and made the final error message emitted by helmfile better.
More concretely, this moves more output from
helmto STDERR and thedebuglog-level.The overall output from
helmfileshould be a bit more cleaner especially forapply,sync,diffand perhaps otherhelmfilesub-commands, too.For example, when one of release failed,
helmfile's final error message now includes the error message from the failedhelmexecution, like seen in the last line:This way you can better understand what caused helmfile to finally fail.
helmfilehas been streaminig a lot of stdout and stderr contents from thehelmcommands regardless of the helmfile's log-level. It has been suppressed by default and moved to thedebuglog-level.You will see that it helps you focus on what was the cause of a failure.
While working on the above, I found an another bug that made
--detailed-exitcodeuseless in some case.That is,
helmfile diff --detailed-exitcode, when any diff existed only in sub-helmfiles, has been returning an exit code of1. It should return2when any release had diff and no release had an error, regardless of the target is a sub-helmfile or a parent helmfile. Why? Because that's what--detailed-exitcodemeant for!After this PR gets merged,
helmfile diff --detailed-exitcodepropery return exit code2in such cases.Fixes #543
Resolves #540