-
Notifications
You must be signed in to change notification settings - Fork 3.9k
fix: update loki-helm-test image tag to latest commit #19227
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
fix: update loki-helm-test image tag to latest commit #19227
Conversation
Fixes grafana#19162 Signed-off-by: puretension <[email protected]>
|
@slim-bean FYI, I assume you might have set this value in the tests back when you were working on updating the Helm charts? |
production/helm/loki/values.yaml
Outdated
| repository: grafana/loki-helm-test | ||
| # -- Overrides the image tag whose default is the chart's appVersion | ||
| tag: "ewelch-distributed-helm-chart-17db5ee" | ||
| tag: "217d263df" |
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.
i think maybe this should just be set to match the same version used for the loki image and enterprise logs image?
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.
You're absolutely right! I've updated the implementation to use .Chart.AppVersion instead of a hardcoded commit hash.
This ensures the loki-helm-test image automatically matches the main loki image version.
Thank you for thefeedback!
- Replace hardcoded commit hash with dynamic Chart.AppVersion reference - Ensures loki-helm-test image version matches main loki image version - Improves version management consistency across the chart Addresses maintainer feedback on grafana#19227 Signed-off-by: puretension <[email protected]>
Resolves security vulnerabilities by updating from outdated feature branch tag to latest. Fixes helm test failures. Fixes grafana#19162 Signed-off-by: puretension <[email protected]>
|
Hi @JStickler, I apologize for the test failure in my previous commit. I've now updated the implementation to use The issue was that Thanks to this debugging process, I gained a deeper understanding of how Helm chart image resolution works and the importance of verifying image availability before implementation. Could you please re-run the merge when you have a chance? The tests should pass now. Thank you for your patience! |
b68ad0c to
3f632fa
Compare
|
Hi @JStickler, I sincerely apologize for the multiple iterations on this PR. I wasn't initially aware that documentation updates were required for Helm chart changes, which caused the previous merge attempts to fail. This experience has taught me how thorough and precise Loki's merge policies are. All changes are now complete:
The PR should now be ready for final merge. Thank you for your patience and guidance throughout this process! 🙏 |
|
@puretension no need to apologize for test failures, nobody expects perfection all the time (that's why we have tests!). |
What this PR does / why we need it:
Updates the loki-helm-test image tag from an outdated feature branch tag (ewelch-distributed-helm-chart-17db5ee) to latest.
This resolves security vulnerabilities in the helm test image identified in issue #19162 and ensures helm tests pass consistently.
Which issue(s) this PR fixes:
Fixes #19162
Special notes for your reviewer:
The previous tag ewelch-distributed-helm-chart-17db5ee was from a contributor's feature branch and contained security
vulnerabilities as reported in #19162. Using latest tag ensures we always use the most recent secure image build and prevents future occurrences of this issue.
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR