search

konstructio / gitops-template

upstream template for your open source gitops repository
MIT License
67 stars 56 forks source link

Preventing the miss-interpretation of the appVersion #662

Closed omar-nahhas closed 10 months ago

omar-nahhas commented 11 months ago

If the shortSha contains an 'e' on the third to the last character, it was misinterpreted by the sed command hence writing an invalid version in the helm chart chart.yaml

For example a shortSha variable, holding a value of "6063e25", was misinterpreted as a numerical value in scientific notation by the system, generating an appVersion of "6.063e+28" instead of appVersion: "6063e25".

The fix is ensuring that shortSha is always treated as a string.

fharper commented 11 months ago

Thanks for your PR @omar-nahhas . Let me ask someone from the engineering team to review the changes, but they look good to me.

fharper commented 11 months ago

@omar-nahhas can you sign your commits please so we can merge your changes? Here is how to do so https://docs.github.com/en/authentication/managing-commit-signature-verification

johndietz commented 11 months ago

confirmed this introduced no regression issues and expect it will string protect the problematic sha values as intended. thank you so much for this contribution @omar-nahhas. planning to include this in the next 2.3 patch release. can i ask you to amend your commit with a gpg-signed commit so we can merge your proposed changes?

marc0olo commented 10 months ago

thanks @omar-nahhas, was just asking about this on slack again 👍

edit: I commented in slack again as I am not sure if this is fixing my issue 🤔 (my shortSha was 0110366)

fharper commented 10 months ago

@omar-nahhas Hey Omar, can I ask you to sign your commits please?

We would like to merge these changes so they can be in our next release, and we want to ensure you get the credits for your help!

If you can't, don't want to, or don't have time, do you mind if I add the same fixes in our code myself? It's an issue we see more, and more, and need to be fixed :)

fharper commented 10 months ago

Thanks for your PR @omar-nahhas 🎉