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.

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 subpath from DeepCopy instead of pointer #580

Merged
merged 7 commits into from
May 30, 2023

Conversation

odinnordico
Copy link
Contributor

@odinnordico odinnordico commented May 24, 2023

Pull request

What this PR does / why we need it

When doing the Merge over WorkloadSpec, on each "sub-Merge", ResetSource (which converts Source = nil) was used but keeping a stash with the previous value of the section (like Source) but as those are pointer references then when ResetSource is called it changes the whole reference, then the validations in others struct members fail.

This PR uses DeepCopy to keep the "stash" so the further validations/checks are against the actual values.

Which issue(s) this PR fixes

Fixes #565

Describe testing done for PR

  • Create a workload from a local source

image

  • Create a file to update the workload

image

  • Update the workload from a file, changing the subpath

image

Additional information or special notes for your reviewer

anibmurthy
anibmurthy previously approved these changes May 24, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 25, 2023

Codecov Report

Merging #580 (181248e) into main (fd35b06) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 181248e differs from pull request most recent head 6668c91. Consider uploading reports for the commit 6668c91 to get more accurate results

@@            Coverage Diff             @@
##             main     #580      +/-   ##
==========================================
+ Coverage   85.76%   85.78%   +0.02%     
==========================================
  Files          68       68              
  Lines        4249     4255       +6     
==========================================
+ Hits         3644     3650       +6     
  Misses        473      473              
  Partials      132      132              
Impacted Files Coverage Δ
pkg/apis/cartographer/v1alpha1/workload_helpers.go 94.87% <100.00%> (+0.07%) ⬆️

@odinnordico odinnordico marked this pull request as ready for review May 25, 2023 16:41
@odinnordico odinnordico marked this pull request as draft May 25, 2023 17:29
@odinnordico odinnordico marked this pull request as ready for review May 26, 2023 14:58
pkg/apis/cartographer/v1alpha1/workload_test.go Outdated Show resolved Hide resolved
pkg/commands/workload_apply_test.go Outdated Show resolved Hide resolved
pkg/commands/workload_apply_test.go Outdated Show resolved Hide resolved
pkg/commands/workload_apply_test.go Outdated Show resolved Hide resolved
warango4
warango4 previously approved these changes May 29, 2023
Copy link
Contributor

@warango4 warango4 left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Diego Alfonso <[email protected]>
@odinnordico odinnordico merged commit 3cedc29 into vmware-tanzu:main May 30, 2023
warango4 added a commit to warango4/apps-cli-plugin that referenced this pull request Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

subpath is being removed when updating lsp or source image workload from file
5 participants