search

koslib / helm-eks-action

The simplest Github Action for executing Helm commands on EKS - cluster authentication included
MIT License
59 stars 61 forks source link

Confusing note about breaking changes #36

Closed adamniedzielski closed 1 year ago

adamniedzielski commented 1 year ago

First and foremost thank you for creating this GitHub action! 💛 We've been using it for a while now and it's been a great help.

I was reviewing recent changes and saw in README:

Breaking change from v2.x and onwards

I checked the current version of the action - 1.25.2. Then I tried to find version 2.x released and couldn't. After looking at the recent commits I realised that 2.x most likely refers to version 2.0 of the image - https://github.com/koslib/helm-eks-action/commit/1e6fdfac3e06f9a6d14a2d1ac96a014de6a0974d and 1.25.2 already contains the breaking change.

Can you confirm if my reasoning is correct? If yes, I think that it would make sense to clarify the note in the README.

koslib commented 1 year ago

Hi @adamniedzielski and thanks for your kind words - also glad you find this action useful!

In retrospect, I find this indeed confusing. To be clear, in version v1.25.2 the base docker image tag has indeed been changed to 2.0. However the action itself does not contain any breaking changes. This is because the KUBE_CONFIG_DATA env var is still needed, and it doesn't matter if it's loaded from the repo secrets or if it's dynamically fed from a previous step setting an env variable key/value.

I see some options moving forward and I'd like your input:

  1. Remove this note as it is causing confusion, and replace it with a note pinpointing to a more secure way of providing the kube context data without having to fill it base64-ed in the repo secrets.
  2. Should the major version of the action and the base docker image be aligned? Now that I am thinking this is kinda makes sense - and I'd appreciate a lot your thoughts!

Btw if you're interested in helping out maintaining/extending the action lmk :)

koslib commented 1 year ago

Hi again @adamniedzielski I've went ahead and closed this issue after attempting to make the readme to look les confusing. Let me know if you think it still has room for improvement in this regard or feel free to submit a PR :) Thanks again!