fix: sanitize artifacthub version string for semver compliance #845
Conversation
Signed-off-by: PARTHIV PRATIM SAIKIA <[email protected]>
Summary of ChangesHello @parthivsaikia, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where version strings from ArtifactHub, when prefixed with a 'v' (e.g., "v2022.06.14"), failed strict semantic versioning validation. By implementing a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an important issue with semver compliance for versions from ArtifactHub by removing the 'v' prefix. The change is straightforward and uses strings.TrimPrefix effectively. My main feedback is regarding testing. While the fix itself is correct, it's not covered by automated tests. I've added a comment suggesting the addition of unit tests to ensure this fix is robust and to prevent future regressions. This will improve the overall quality and maintainability of the code.
generators/artifacthub/package.go
Outdated
| } | ||
| comp.Model.Metadata.AdditionalProperties["source_uri"] = pkg.ChartUrl | ||
| comp.Model.Version = pkg.Version | ||
| comp.Model.Version = strings.TrimPrefix(pkg.Version, "v") |
There was a problem hiding this comment.
This change correctly sanitizes the version string. However, there are no automated tests to verify this fix and prevent future regressions.
The existing tests in package_test.go do not seem to cover this logic, as the AhPackage structs used for testing don't have the Version field populated.
Please consider adding a unit test for the GenerateComponents method that specifically verifies the version string sanitization for different cases, such as:
- Version with a
vprefix (e.g.,v1.2.3) - Version without a
vprefix (e.g.,1.2.3) - Date-based versions with a
vprefix (e.g.,v2022.06.14) - An empty version string
Adding tests will ensure the stability of this fix. Note that GenerateComponents might need some refactoring to be more easily unit-testable, for example by mocking its dependency on manifests.GetCrdsFromHelm.
Signed-off-by: PARTHIV PRATIM SAIKIA <[email protected]>
|
Thank you for your contribution! @parthivsaikia 🎉 Let's discuss this PR during the Meshery Development Meeting today. Meshery Development Meeting | December 10th, 2025 |
leecalcote
left a comment
leecalcote
left a comment
There was a problem hiding this comment.
@parthivsaikia, thanks for this. Will you provide numbers showing the before and after here? I'm concerned that while this change will be helpful to some models that it will be detrimental to others...
|
@leecalcote There are total 5 models affected by this change. Those are:
And this change doesn't introduce any detrimental effect to the other models. |
3 participants
Description
This PR updates the ArtifactHub generator to sanitize upstream version strings. Specifically, it uses strings.TrimPrefix to remove the leading v (e.g., converting v2022.06.14 to 2022.06.14) before assigning it to the model version. This ensures compliance with the strict Semantic Versioning checks enforced by the Masterminds/semver library and resolves validation errors for valid upstream charts.
This PR fixes #843
Notes for Reviewers
Verified locally that the fix properly strips the prefix.
The change is isolated to generators/artifacthub/package.go.
Signed commits