fix(argo-rollouts): plugin block rendering was incorrect

argoproj / argo-helm

ArgoProj Helm Charts

https://argoproj.github.io/argo-helm/

Apache License 2.0

1.77k stars 1.88k forks source link

fix(argo-rollouts): plugin block rendering was incorrect #3014

Closed taer closed 2 weeks ago

taer commented 2 weeks ago

Resolves https://github.com/argoproj/argo-helm/issues/2957

Checklist:

[X] I have bumped the chart version according to versioning
[X] I have updated the documentation according to documentation
[X] I have updated the chart changelog with all the changes that come with this pull request according to changelog.
[X] Any new values are backwards compatible and/or have sensible default.
[X] I have signed off all my commits as required by DCO.
[X] My build is green (troubleshooting builds).

the config map was getting rendered as

data:
  - location: https://github.com/argoproj-labs/rollouts-plugin-trafficrouter-gatewayapi/releases/download/v0.4.0/gatewayapi-plugin-linux-arm64
    name: argoproj-labs/gatewayAPI

this changes it to

data:
  trafficRouterPlugins: |-
    - location: https://github.com/argoproj-labs/rollouts-plugin-trafficrouter-gatewayapi/releases/download/v0.4.0/gatewayapi-plugin-linux-arm64
      name: argoproj-labs/gatewayAPI

as shown in metricPlugin and trafficPlugin

michel-gleeson commented 1 week ago

@taer thanks for fixing this!

michel-gleeson commented 1 week ago

Commenting in here as there's a few things I want to update with:

thanks again to everyone for the change and the review!
there is still an inconsistency between the documented helm values and this fix (the documented method suggests passing in stringified yaml, but this change does the conversion to string for us)
our package updater bot picked this up as a non-breaking change (since it was added to a patch change) but since I was working around the issue, the change broke our rollout pods

Could we do something to address 2?

taer commented 1 week ago

3. our package updater bot picked this up as a non-breaking change (since it was added to a patch change) but since I was working around the issue, the change broke our rollout pods

First, so sorry. It was totally a breaking change, but the original was broken, assumed it was safe. I assume you were getting creative with the parameter in the helm values?

My helm values.yaml has something like this in it

controller:
  trafficRouterPlugins:
    - name: "argoproj-labs/gatewayAPI"
      location: "https://github.com/argoproj-labs/rollouts-plugin-trafficrouter-gatewayapi/releases/download/v0.4.0/gatewayapi-plugin-linux-arm64"
      sha256: "07e2fe515c900899c96777aac0ab490437a3d62c8adb94512ccb8b859325eb11"

which OMG, isn't what that example shows. Subtle.

I feel like fixing the example would be the cleaner method. Makes it feel more real.

Also, what is this bot you use? That sounds interesting.

taer commented 1 week ago

I opened another PR here: https://github.com/argoproj/argo-helm/pull/3036 What do you think @michel-gleeson ?

Also, I also broke our rollouts w/ this change. I forgot to update my hacked version of values.yaml that was working around it, and it restarted while I was out on vacation. :)