Skip to content

fix: op patching to not add elements if key already exists#2206

Merged
FabianKramm merged 2 commits intomasterfrom
fix-op-patching-adds
Aug 9, 2022
Merged

fix: op patching to not add elements if key already exists#2206
FabianKramm merged 2 commits intomasterfrom
fix-op-patching-adds

Conversation

@carlmontanari
Copy link
Copy Markdown
Contributor

What issue type does this pull request address? (keep at least one, remove the others)
/kind bugfix
/kind test

What does this pull request do? Which issues does it resolve? (use resolves #<issue_number> if possible)
N/A

Please provide a short message that should be published in the DevSpace release notes
Fixed an issue where DevSpace add patch operations on existing keys breaks patching.

What else do we need to know?

This fixes an issue where adding an already existing key to a map would basically append the value in your patch to the key. Unfortunately this would basically shift the yaml around so now the previous value would be the key on the next line. Example:

   importme:
        build:
            buildKit: {}
        context: ../..
        veryimportantpath: dockerfile
        ./Dockerfile: image
        golang:1.17: rebuildStrategy

The above output is an example where there was a context set to "veryimportantpath", and an add op setting the context to "../..". This would obviously cause some errors. Moreover subsequent operations may try to reference the key "dockerfile" which is actually no longer a key in the document, causing an exception like remove operation does not apply: doc is missing path: images.agent.dockerfile.

The Perform function got a little more complicated than I'd like to account for not breaking the ability to add elements into existing arrays.

@netlify
Copy link
Copy Markdown

netlify Bot commented Aug 5, 2022

Deploy Preview for devspace-docs canceled.

Name Link
🔨 Latest commit 70af82f
🔍 Latest deploy log https://app.netlify.com/sites/devspace-docs/deploys/62f1955dfbdd6900087b9210

Copy link
Copy Markdown
Collaborator

@FabianKramm FabianKramm left a comment

Choose a reason for hiding this comment

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

@carlmontanari thanks for the PR! Please take a look at my comments below

Comment thread pkg/devspace/config/loader/patch/operation.go Outdated
Comment thread pkg/devspace/config/loader/patch/operation.go Outdated
Comment thread pkg/devspace/config/loader/patch/operation.go Outdated
Copy link
Copy Markdown
Collaborator

@FabianKramm FabianKramm left a comment

Choose a reason for hiding this comment

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

@carlmontanari thanks! LGTM!

@FabianKramm FabianKramm merged commit 77e3cd2 into master Aug 9, 2022
@FabianKramm FabianKramm deleted the fix-op-patching-adds branch August 9, 2022 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants