Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(controller): when updating a values.yaml, make sure image tags that look like a number are still treated as strings #1315

Merged
merged 2 commits into from
Dec 22, 2023
Merged

fix(controller): when updating a values.yaml, make sure image tags that look like a number are still treated as strings #1315

merged 2 commits into from
Dec 22, 2023

Conversation

maksimstankevic
Copy link
Contributor

@maksimstankevic maksimstankevic commented Dec 21, 2023 •
edited by krancour
Loading

Fixes #876

image

@maksimstankevic
https://github.com/akuity/kargo/issues/876
70987a2 
Signed-off-by: Maksim Stankevic <maksim.stankevic1@gmail.com>
@maksimstankevic maksimstankevic requested a review from a team as a code owner December 21, 2023 20:22
@netlify Netlify
Copy link

netlify bot commented Dec 21, 2023 •
edited
Loading

❌ Deploy Preview for docs-kargo-akuity-io failed.

Name Link
🔨 Latest commit df36c0f
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/6585563cf6cc2d00080dd703

@maksimstankevic maksimstankevic changed the title https://github.com/akuity/kargo/issues/876 fix: https://github.com/akuity/kargo/issues/876 Dec 21, 2023
@maksimstankevic maksimstankevic mentioned this pull request Dec 21, 2023
Closed
krancour
krancour reviewed Dec 21, 2023
View reviewed changes
internal/controller/promotion/helm.go Outdated
@@ -125,7 +125,7 @@ func buildValuesFilesChanges(
tagsByImage := map[string]string{}
digestsByImage := make(map[string]string, len(images))
for _, image := range images {
tagsByImage[image.RepoURL] = image.Tag
tagsByImage[image.RepoURL] = "'" + image.Tag + "'"
Copy link
Member

Choose a reason for hiding this comment

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

This will break things farther down on lines 155 to 161.

Just change line 160.

krancour
krancour reviewed Dec 21, 2023
View reviewed changes
internal/controller/promotion/helm_test.go Outdated
@@ -321,8 +321,8 @@ func TestBuildValuesFilesChanges(t *testing.T) {
t,
map[string]map[string]string{
"fake-values.yaml": {
"fake-key": "fake-url:fake-tag",
"second-fake-key": "second-fake-tag",
"fake-key": "fake-url:'fake-tag'",
Copy link
Member

Choose a reason for hiding this comment

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

This test case demonstrates the problem I mentioned here.

fake-url:'fake-tag' isn't a valid image identifier.

Copy link
Contributor Author

@maksimstankevic maksimstankevic Dec 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@krancour thank you, I fixed it df36c0f
I still had to keep single quotes in the tagOnly test case as otherwise it fails, is it OK?

changeSummary is good now:
image
and tag correctly quoted in values:
image

@maksimstankevic
resolving akuity#1315 (comment)
df36c0f 
Signed-off-by: Maksim Stankevic <maksim.stankevic1@gmail.com>
@krancour krancour self-requested a review December 22, 2023 17:31
krancour
krancour approved these changes Dec 22, 2023
View reviewed changes
Copy link
Member

@krancour krancour left a comment

Choose a reason for hiding this comment

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

Thank you @maksimstankevic!

@krancour krancour enabled auto-merge December 22, 2023 17:32
@krancour krancour changed the title fix: https://github.com/akuity/kargo/issues/876 fix(controller): when updating a values.yaml, make sure image tags that look like a number are still treated as strings Dec 22, 2023
@krancour krancour self-assigned this Dec 22, 2023
@codecov Codecov
Copy link

codecov bot commented Dec 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e3e2c1f) 48.87% compared to head (df36c0f) 45.82%.
Report is 30 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1315      +/-   ##
==========================================
- Coverage   48.87%   45.82%   -3.05%     
==========================================
  Files         127      135       +8     
  Lines        9763    11556    +1793     
==========================================
+ Hits         4772     5296     +524     
- Misses       4831     6069    +1238     
- Partials      160      191      +31

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@krancour krancour added this to the v0.4.0 milestone Dec 22, 2023
@krancour krancour added this pull request to the merge queue Dec 22, 2023
Merged via the queue into akuity:main with commit d50f002 Dec 22, 2023
16 of 21 checks passed
@maksimstankevic maksimstankevic deleted the issue/876 branch February 23, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krancour krancour krancour approved these changes

Assignees

@krancour krancour

@maksimstankevic maksimstankevic

Labels
area/controller kind/bug priority/normal
Projects
None yet
Milestone
v0.4.0
Development

Successfully merging this pull request may close these issues.

When Kargo Updates Helm Values ,is not seting quote on the tag value
2 participants
@maksimstankevic @krancour