fix(helm): support digest-pinned image tags in values.yaml#14907
Open
austb-cloudbolt wants to merge 3 commits intodependabot:mainfrom
Open
fix(helm): support digest-pinned image tags in values.yaml#14907austb-cloudbolt wants to merge 3 commits intodependabot:mainfrom
austb-cloudbolt wants to merge 3 commits intodependabot:mainfrom
Conversation
The Helm image updater's `update_image_tags_recursive` was using
`split("\n")` without a `-1` limit, so a trailing newline was silently
dropped on the round-trip back to the joined string. That dropped
newline made the post-update content compare unequal to the input even
when no tag scalar had actually been replaced, defeating the
"Expected content to change!" guard.
The visible symptom is that Dependabot opens PRs whose only diff is a
trailing-newline change on values.yaml, even when no version was
actually bumped.
Switch to `split("\n", -1)` so the round-trip is identity. Update the
"with complex YAML structure containing arrays and maps" spec, which
was relying on the buggy round-trip to silently no-op (arrays of tags
aren't a real Helm pattern), and add a regression test covering the
trailing-newline + no-match case.
`update_version_tags` matched the YAML tag scalar with strict equality against `req[:source][:tag]`, so values shaped like `tag: "v1.6.41@sha256:..."` -- where the parser splits the string into a `:tag` and a `:digest` -- could never be matched. The updater would silently no-op, and the trailing-newline guard fix in the previous commit would surface that as a "Expected content to change!" error. Recognize both shapes: the bare tag and the digest-pinned form (`tag@digest`). Replace the tag substring within the full YAML scalar, preserving the digest portion (and any trailing comment on the line -- the previous `gsub(tag, version)` against the whole line would clobber lines containing `tag` elsewhere). The digest is left intact in this commit. We don't have a fresh digest to swap in here, so leaving the existing one in place keeps the user's pin visible (now mismatched, but the user can refresh it manually); a follow-up commit wires up the docker UpdateChecker to resolve a new digest so the two land in lockstep. Adds spec coverage for quoted, unquoted, and trailing-comment forms.
austb-cloudbolt
force-pushed
the
helm-digest-pinned-tags
branch
3 times, most recently
from
8528da8 to
344a9ff
Compare
Author
|
Tests seem to be having problems connecting to one of Ubuntu's servers If a maintainer sees this and wants to re-run failed tests, please do, otherwise I will refresh the commit later and have them try again. |
The previous commit got the tag updating cleanly for `tag@digest`
values, but left the digest portion untouched. That makes the
resulting PR functionally broken when merged: Docker resolves
`tag@digest` references by digest, so the new tag and the preserved
old digest still pull the old image. Users had to manually refresh
the digest after every Dependabot PR.
Hook the existing Docker UpdateChecker -- which already resolves
manifest digests for tag bumps -- into the Helm flow so digest-pinned
Helm values get a fresh digest alongside the bumped tag.
* `Helm::UpdateChecker#build_docker_dependency` now propagates the
parser's `:digest` into the docker dependency. Docker's
`updated_requirements` only resolves a new digest when the source
has one (`if digest || pin_digests?`), so without this it would
never look up a fresh digest.
* `Helm::UpdateChecker#docker_checker` is now a memoized helper.
Previously the checker was built once for the version lookup and
discarded; now it's shared with `#fetch_new_image_digest` so we
don't pay for two round-trips against the registry. The docker
checker memoizes `digest_of` internally, so a single instance is
enough.
* `Helm::UpdateChecker#updated_requirements` now follows dependabot's
convention for `:docker_image` requirements: write the new tag and
new digest into `:source` (so `previous_requirements` keeps the old
values for the file updater to match against). `:helm_chart`
requirements are unchanged.
* `Helm::UpdateChecker#updated_image_source` raises
DependencyFileNotResolvable when the source is digest-pinned and a
new digest can't be resolved. This is a defensive guard: in
practice the docker checker treats "couldn't resolve a digest" as
"up-to-date" so `#latest_version` returns nil and we never reach
this path. But if that contract ever changes, emitting a tag-only
bump on a digest-pinned source would produce a PR that still pulls
the old image -- fail loudly rather than produce a misleading diff.
* `Helm::FileUpdater::ImageUpdater#update_version_tags` now reads the
OLD tag/digest from `previous_requirements` and the NEW tag/digest
from `requirements`, replacing both in lockstep within the YAML
scalar.
Updates the spec convention so `dependency_requirements` carries the
new tag/digest and `dependency_previous_requirements` carries the old.
The "wrong-tag" tests now override `dependency_previous_requirements`
(matching the side the file updater reads from). The digest-pinned
file_updater tests assert both tag and digest are replaced, and a new
update_checker context covers the docker-image source-rewrite end to
end against a stubbed registry.
austb-cloudbolt
force-pushed
the
helm-digest-pinned-tags
branch
from
344a9ff to
18b586e
Compare
Author
|
Tests look good after a re-run |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are you trying to accomplish?
Dependabot detects an update is available, opens a PR, but the only diff is a trailing-newline change on
values.yaml— the tag is never actually bumped. Two compounding bugs and one missing feature were responsible:update_image_tags_recursivewas splitting and rejoining content on"\n"without a-1limit, silently dropping a trailing newline. That made the post-update content compare unequal to the input even when no tag scalar had been replaced, defeating the"Expected content to change!"guard. The user-visible result is a "successful" PR with a meaningless diff.The Helm parser splits a value like
"v1.6.41@sha256:..."into:tag("v1.6.41") and:digest("sha256:..."), so the scalar in the YAML —"v1.6.41@sha256:..."— never equals the bare:tagvalue. The find returned nil, no replacement happened, and bug Bump eslint-plugin-prettier from 2.0.1 to 2.1.1 in /helpers/javascript #1 then masked it as a non-trivial diff.Even after fixing the matching, leaving the old digest in place would produce a misleading PR — Docker resolves
tag@digestreferences by digest, so the new pull would still resolve to the old image while the YAML claimed otherwise. The DockerUpdateCheckeralready resolves manifest digests for tag bumps; we just weren't wiring it through.After all three commits, the example above becomes:
Anything you want to highlight for special attention from reviewers?
Docker's
UpdateChecker#digest_up_to_date?already treats "couldn't resolve a digest" as "up-to-date" for digest-pinned sources — defensively, since emitting a tag-only bump ontag@digestwould produce a PR that still pulls the old image. Following that convention, when the helm checker delegates and the docker checker reports up-to-date,Helm::UpdateChecker#latest_versionreturnsnilandupdated_requirementsshort-circuits, leaving the source untouched.How will you know you've accomplished your goal?
I added tests for both the update checker and the image updater that test the behavior with a stubbed registry response.
Checklist