-
Notifications
You must be signed in to change notification settings - Fork 157
ROX-28901: Derive EARLIER_CHART_VERSION in scanner-v4-install tests #15594
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mclasmeier - I've reviewed your changes - here's some feedback:
- Adjust the grep and sed regex to support multi-digit patch versions (e.g. use '[0-9]+' for the z component) so tags like '1.10.12' aren’t dropped.
- Use a version-aware sort (e.g.
sort -V
) instead of numeric sort on the x.y string to correctly order multi-digit versions. - Add explicit error handling or a clear fallback when no previous x.y.0 tag exists to avoid silent failures in the test.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Images are ready for the commit at f4e5872. To use with deploy scripts, first |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #15594 +/- ##
==========================================
- Coverage 48.80% 48.79% -0.01%
==========================================
Files 2590 2590
Lines 190492 190492
==========================================
- Hits 92963 92960 -3
- Misses 90231 90233 +2
- Partials 7298 7299 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
50def51
to
9ace8d9
Compare
f8d517e
to
74bb9bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compared to the way we figure out the previous version in the operator Makefile, this purely git-based method has the advantage that it's mostly offline, but it also has the disadvantage that it will generate an unusable version in the time frame between the x.y.0
tag is pushed to the git repo, to the moment the installation artifacts (images and perhaps also helm charts) are built and pushed.
Given that we need network connectivity anyway for this test to work, I think it would be preferable to use the image registry to check for previous version existence, than have a way which is offline but fails every now and then...
@porridge, see my last commit, I think I was able to increase reliability while also retaining the general approach. Instead of using git tags and then hoping that this will allow the test suite to resolve the computed tag to resolve to a Helm chart version, I figured it would make much more sense to begin with Helm chart versions, because that is what is needed here. So, the code now inspects the |
/retest |
@sourcery-ai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mclasmeier - I've reviewed your changes - here's some feedback:
- Consider extracting the new version‐resolution helpers (resolve_previous_x_y_0_version, list_sub_directories, etc.) into a shared test‐utility file to keep the main .bats script concise.
- To speed up CI and reduce overhead, consider caching the cloned helm‐charts repo between runs or using a more focused shallow clone (e.g. limit tags) instead of pulling the full repo each time.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new functions are rather complicated, so it would be great if there were some unit tests covering corner cases like when there exist previous patch versions or no patch version for the given minor version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
ebe6899
to
df14df5
Compare
@mclasmeier: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Description
Don't hard-code an older version, instead derive a previous version from the existing git tags.
User-facing documentation
Just a change in tests, nothing user-facing.
Testing and quality
Automated testing
How I validated my change
From the CI log:
which is correct (given that Helm charts fro 4.8.x have not yet been released).