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: skaffold diagnose outputs incorrect yaml for multiconfig projects #5531

Merged

Conversation

gsquared94
Copy link
Contributor

@gsquared94 gsquared94 commented Mar 11, 2021

skaffold diagnose --yaml-only used on a multiconfig project outputs a merged list of all resolved configs.
For example:
skaffold.yaml:

...
requires:
  - path: ./skaffold2.yaml
build:
  - image: foo1
...

skaffold2.yaml:

...
build:
  - image: foo2
...

should produce:

...
build:
  - image: foo1
...
---
...
build:
  - image: foo2
...

But it's actually producing:

...
requires:
  - path: ./skaffold2.yaml
build:
  - image: foo1
...
---
...
build:
  - image: foo2
...

If this output yaml is used as the input for another skaffold run then the skaffold2.yaml artifacts are duplicated since it's reimported.
This PR fixes this issue by removing the dependency list prior to marshalling.

@gsquared94 gsquared94 requested a review from a team as a code owner March 11, 2021 02:31
@google-cla google-cla bot added the cla: yes label Mar 11, 2021
@gsquared94 gsquared94 requested a review from nkubala March 11, 2021 02:35
@gsquared94 gsquared94 changed the title fix: diagnose yaml output not reusable for multiconfig projects fix: diagnose yaml output not usable for multiconfig projects Mar 11, 2021
@gsquared94 gsquared94 changed the title fix: diagnose yaml output not usable for multiconfig projects fix: skaffold diagnose outputs incorrect yaml for multiconfig projects Mar 11, 2021
@codecov
Copy link

codecov bot commented Mar 11, 2021

Codecov Report

Merging #5531 (bf42cab) into master (3160270) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5531   +/-   ##
=======================================
  Coverage   71.32%   71.33%           
=======================================
  Files         400      400           
  Lines       14831    14832    +1     
=======================================
+ Hits        10578    10580    +2     
  Misses       3475     3475           
+ Partials      778      777    -1     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/diagnose.go 33.33% <0.00%> (-1.45%) ⬇️
pkg/skaffold/docker/parse.go 82.16% <0.00%> (-1.09%) ⬇️
pkg/skaffold/util/tar.go 56.00% <0.00%> (+5.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3160270...bf42cab. Read the comment docs.

Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

nice, thanks for jumping on this so quickly! tried it out and it works perfectly

@nkubala nkubala merged commit c1c322a into GoogleContainerTools:master Mar 11, 2021
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.

2 participants