search

opendevstack / ods-pipeline

Alternative ODS CI/CD pipeline based on Tekton / OpenShift Pipelines
Apache License 2.0
13 stars 5 forks source link

Should install script with dry-run option show diff against deployed? #569

Closed henrjk closed 2 years ago

henrjk commented 2 years ago

Here is my scenario, I was aiming to add a sidecar to one of the build tasks.

To check whether my helm changes look good I used the following command:

./install.sh -n <project-cd-namespace> --dry-run -f values.yaml,secrets.yaml

The output looked wrongly indented to me. So I wanted to test this by first making a manual change in the console, run a pipeline using the manual modified task and then make sure that helm sees no diff anymore.

Following this plan I discovered that helm only shows the diff between the current state of helm definitions and what it had installed not what is actually deployed now. There is issue helm/helm#8718 about this which eventually led me to https://github.com/databus23/helm-diff/issues/176#issuecomment-1008739218.

When I change the command issued by the above's command by adding the suggested --three-way-merge like this:

helm -n <project-cd-namespace> secrets diff --three-way-merge upgrade --install --detailed-exitcode --values=values.yaml --values=secrets.yaml ods-pipeline ./ods-pipeline

The diff now showed my manual changes in the existing task.

Should this be what the command displays by default? Should there be an option to enable the additional --three-way-merge?

michaelsauter commented 2 years ago

Interesting! I need to learn a bit more about this flag and what it means in detail but from what you write it should probably be the default?

michaelsauter commented 2 years ago

I've done some tests. This should definitely be the default in the install script, but also in the Helm task (https://github.com/opendevstack/ods-pipeline/blob/master/cmd/deploy-with-helm/main.go#L373-L380). Would you have time to add it?

michaelsauter commented 2 years ago

@henrjk I wonder whether it would make sense to combine this issue with #574? There I am contemplating to have just one task parameter (upgrade-flags), but maybe we should instead repurpose the other other task parameter (diff-flags) to set additional flags for the diff call. This flag could then default to --three-way-merge, allowing to adjust if needed. Other flags that diff supports that may be interesting to set optionally are e.g. --include-tests, --no-hooks. Thoughts?

henrjk commented 2 years ago

The current behavior allows to make manual changes on the resources which would not be reverted as soon as a build runs. I could imagine that some would like to be able to work like this. To me it appears less confusing if the helm charts are compared against current infrastructure version as proposed. Either way should the behavior be documented in an ADR? I am not certain whether I find the time to work on the changes.

henrjk commented 2 years ago

Ok I did some more reading. The example given in https://github.com/helm/helm/issues/9937#issuecomment-952529143 is good as it shows that a manual change is overwritten with the destination state from helm, whereas an addition is left alone. And the examples in the helm documentation also describe this. https://github.com/helm/helm/issues?q=is%3Aissue+upgrade+manual+change+ shows that there are many edge cases. One suggestion is that it can be useful to use the -atomic option (see https://github.com/helm/helm/issues/10363#issuecomment-976994699). My current thinking is that we don't want to overpromise on aiming to describe the behavior with an ADR and rather provide options to use with helm.

but maybe we should instead repurpose the other other task parameter (diff-flags) to set additional flags for the diff call. This flag could then default to --three-way-merge, allowing to adjust if needed. Other flags that diff supports that may be interesting to set optionally are e.g. --include-tests, --no-hooks. Thoughts?

The proposal to repurpose the flags makes sense to me at the moment. This way the user can use special helm options that may address some of the corner cases in the future.

michaelsauter commented 2 years ago

I agree that the whole topic of what gets updated can be quite confusing. However, I think this issue here is more about your discovery that the helm-diff plugin does not use the same mechanism as Helm itself right now, which is even more confusing. Therefore I think --three-way-merge is pretty much always what one wants, since then at least what the diff shows and what Helm does matches (hopefully!).

As far as I understand, Helm 3 always does a 3-way merge, and that's the best thing there currently is, even though it can be surprising, confusing, or even "incorrect".

One suggestion is that it can be useful to use the -atomic option (see https://github.com/helm/helm/issues/10363#issuecomment-976994699).

When I created the task I made --wait the default but not --atomic. I am not sure if atomic is really a default everyone would expect.

The proposal to repurpose the flags makes sense to me at the moment. This way the user can use special helm options that may address some of the corner cases in the future.

I would implement my proposal then. The current flag upgrade-flags can be used like it is now to control the flags passed to Helm, allowing e.g. to set --atomic if desired. The upgrade-flags would also be passed to the diff command, which going forward would ignore unknown flags. The new diff-flags can be used to set other flags specifically for diff. By default it would be --three-way-merge so that the mechanism of helm-diff and helm is the same, but users can add more flags or even remove --three-way-merge if they want for whatever reason.

For the install.sh script for which you reported the unexpected behaviour, my proposal is to simply hardcode --three-way-merge in the script to have the diff behave the same way as the upgrade. If someone really does not want that, they can just run helm manually with the flags they want.