Skip to content

updating helm version regex for more support #875

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

BlankCanvasStudio
Copy link

@BlankCanvasStudio BlankCanvasStudio commented Feb 5, 2025

SUMMARY

Not all versions of helm output their version info with the format:

version.BuildInfo{Version:"vX.X.X", ....}

some (specifically 3.16.4) use:

version.BuildInfo{Version:"X.X.X", ....}

This causes this function to return None, instead of the version string. I've update the regex to support either format.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

This is an update to plugins/module_utils/helm.py

ADDITIONAL INFORMATION

When using the output of get_helm_version to create a LooseVersion:

https://github.com/ansible-collections/kubernetes.core/blob/main/plugins/module_utils/_version.py#L306

If the version does not match the regex, None will be returned, but the object will still be created.

If the object is then compared with a pinned version, like in this example:

https://github.com/ansible-collections/kubernetes.core/blob/main/plugins/modules/helm.py#L808

without verifying the "version" attribute exists (ie verifying get_helm_version returned a string), the program will crash.

LooseVersion could also be updated to throw an error when None is passed to its constructor, get_helm_version could be updated to throw an error when the program can't regex for the system version, and/or a series of type checks could be added to the program wherever get_helm_vesion is called.

Copy link

Copy link
Member

@gravesm gravesm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gravesm
Copy link
Member

gravesm commented Feb 6, 2025

Ah, also this will need a changelog fragment. Ignore the failing linters for now. #873 should fix those.

@BlankCanvasStudio
Copy link
Author

@gravesm I just added a unit test. It seems from the unit tests that returning None from get_helm_version() is intentional behavior, but there aren't any type checks on its result in the code base. Is this the intended behavior? This issue will keep popping up unless either get_helm_version explicitly creates an error or if type checks are added. I personally think it should generate an error, but I'm wondering why that wasn't the design choice when this was built. I feel like I'm missing something.

@BlankCanvasStudio
Copy link
Author

@gravesm I'm also confused about adding a changelog fragment. The docs you link say I should add a file to changelogs/fragments/, but I don't see that directory in the repository. Are they included from somewhere else? Further, should I also do a version bump in changelog.yaml?

Copy link

@yurnov
Copy link
Contributor

yurnov commented Feb 6, 2025

Hi @BlankCanvasStudio,

@gravesm I'm also confused about adding a changelog fragment. The docs you link say I should add a file to changelogs/fragments/, but I don't see that directory in the repository. Are they included from somewhere else? Further, should I also do a version bump in changelog.yaml?

Regarding changelog fragments, just create this folder and place a fragment for your change, you can use 9bdb9a2 as the example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants