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

Clarify usage of ArtifactOverrides, ImageStrategy #4487

Merged
merged 2 commits into from
Sep 10, 2020
Merged

Clarify usage of ArtifactOverrides, ImageStrategy #4487

merged 2 commits into from
Sep 10, 2020

Conversation

TBBle
Copy link
Contributor

@TBBle TBBle commented Jul 17, 2020

Related: #500, specifically addressing this comment

Description
ArtifactOverrides was incorrectly claiming it needed "values file" syntax, i.e. structured YAML, but it actually takes --set-string syntax, i.e. dotted path.

Also tied ArtifactOverrides directly to ImageStrategy, since the two work together.

Follow-up Work

I didn't check that ImageStrategy is meaningfully documented, so it might need a closer look too.

@TBBle TBBle requested a review from a team as a code owner July 17, 2020 05:29
@TBBle TBBle requested a review from tejal29 July 17, 2020 05:29
Paul "Hampy" Hampson added 2 commits July 23, 2020 16:14
ArtifactOverrides was incorrectly claiming it needed "values file" syntax, i.e. structured YAML, but it actually takes `--set-string` syntax, i.e. dotted path.

Also tied ArtifactOverrides directly to `ImageStrategy`, since the two work together.

Signed-off-by: Paul "Hampy" Hampson <[email protected]>
Signed-off-by: Paul "Hampy" Hampson <[email protected]>
@codecov
Copy link

codecov bot commented Jul 23, 2020

Codecov Report

Merging #4487 into master will increase coverage by 1.84%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4487      +/-   ##
==========================================
+ Coverage   72.39%   74.24%   +1.84%     
==========================================
  Files         334      338       +4     
  Lines       12992    14682    +1690     
==========================================
+ Hits         9406    10901    +1495     
- Misses       2987     3136     +149     
- Partials      599      645      +46     
Impacted Files Coverage Δ
pkg/skaffold/schema/latest/config.go 100.00% <ø> (ø)
cmd/skaffold/app/cmd/render.go 36.36% <0.00%> (-6.50%) ⬇️
pkg/skaffold/test/test.go 95.40% <0.00%> (-4.60%) ⬇️
cmd/skaffold/app/cmd/deploy.go 62.50% <0.00%> (-4.17%) ⬇️
pkg/skaffold/yamltags/tags.go 96.46% <0.00%> (-3.54%) ⬇️
pkg/skaffold/deploy/kubectl/cli.go 88.39% <0.00%> (-2.52%) ⬇️
cmd/skaffold/app/cmd/run.go 52.38% <0.00%> (-0.57%) ⬇️
pkg/diag/validator/pod.go 2.39% <0.00%> (-0.42%) ⬇️
cmd/skaffold/app/signals.go 100.00% <0.00%> (ø)
pkg/skaffold/build/build.go 0.00% <0.00%> (ø)
... and 50 more

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 1110eb1...f02a302. Read the comment docs.

@gsquared94
Copy link
Contributor

Thanks for your contribution @TBBle. However this same change has already been checked in via a different PR so I'll close this one.

@gsquared94 gsquared94 closed this Aug 5, 2020
@TBBle
Copy link
Contributor Author

TBBle commented Aug 7, 2020

@gsquared94 Can you tell which PR these changes are now in? I don't see them in the master branch, i.e. the ArtifactOverrides and ImageStrategy docs do not have my changes.

@gsquared94
Copy link
Contributor

My bad, #4503 fixed the mistake in documentation for ArtifactOverrides only.

@gsquared94 gsquared94 reopened this Aug 7, 2020
@TBBle
Copy link
Contributor Author

TBBle commented Aug 7, 2020

Ah yes. I'd already rebased on #4503.

@TBBle
Copy link
Contributor Author

TBBle commented Aug 7, 2020

Oops, and in the meantime, v2beta6 was released, so to pass CI I need to create a v2beta7. It seems a shame that I need to do that just for documentation updates.

So I'll sit on this for now, and once there's an actual v2beta7 config schema, rebase onto that.

@MarlonGamez
Copy link
Contributor

hey! Just wanted to follow up and let you know that there's a v2beta7 now :)

@tejal29
Copy link
Member

tejal29 commented Aug 28, 2020

This change is a doc only change for the schema.
Its ok to merge this PR in.

@tejal29 tejal29 merged commit 677d665 into GoogleContainerTools:master Sep 10, 2020
@TBBle TBBle deleted the clarify_artifactoverrides_docs branch September 11, 2020 07:35
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.

5 participants