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

docs: add overrides and jobManifestPath to verify docs #8762

Merged
merged 1 commit into from
May 19, 2023

Conversation

maggieneterval
Copy link
Contributor

Fixes: #8743

Description
First pass at improving skaffold verify documentation:

  • Clarify the difference between local and Kubernetes execution modes.
  • Specify how to customize Kubernetes test Jobs using the new overrides and jobManifestPath options.

@maggieneterval
Copy link
Contributor Author

@dorbin PTAL - thanks!

@maggieneterval
Copy link
Contributor Author

@dorbin and @aaron-prindle #8743 also requests that jobManifestPath and overrides be included in the YAML schema documentation, but I am already seeing them show up for https://skaffold.dev/docs/references/yaml/?version=v4beta4#verify and https://skaffold.dev/docs/references/yaml/?version=v4beta5. That said, they don't show up for the default (latest stable?) version of the YAML schema page. Wanted to confirm that was expected. Thanks!

@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #8762 (e7fabe5) into main (290280e) will decrease coverage by 6.07%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #8762      +/-   ##
==========================================
- Coverage   70.48%   64.41%   -6.07%     
==========================================
  Files         515      617     +102     
  Lines       23150    31171    +8021     
==========================================
+ Hits        16317    20079    +3762     
- Misses       5776     9589    +3813     
- Partials     1057     1503     +446     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/completion.go 13.04% <0.00%> (-1.25%) ⬇️
cmd/skaffold/app/cmd/config/list.go 65.21% <ø> (ø)
cmd/skaffold/app/cmd/config/set.go 88.72% <ø> (ø)
cmd/skaffold/app/cmd/config/util.go 54.28% <ø> (ø)
cmd/skaffold/app/cmd/credits.go 100.00% <ø> (ø)
cmd/skaffold/app/cmd/credits/export.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/deploy.go 40.90% <0.00%> (-12.94%) ⬇️
cmd/skaffold/app/cmd/generate_pipeline.go 60.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_modules.go 65.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_profiles.go 66.66% <ø> (ø)
... and 40 more

... and 407 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -12,13 +12,15 @@ verify:
name: integration-test-container
image: integration-test-container
executionMode:
kubernetesCluster: {}
kubernetesCluster:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be commented out w/ some indication the field is optional?

Copy link
Contributor Author

@maggieneterval maggieneterval May 10, 2023

Choose a reason for hiding this comment

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

Good idea. I made a couple changes to help clarify that these fields are optional:

  1. Changed wording on line 28 to "There are two ways to optionally customize the Skaffold-generated Kubernetes Job"
  2. Added "optional" to describe both override configuration options on lines 37 and 38
  3. Commented out the optional lines in the YAML and added "[optional]" above to further clarify.

Let me know what you think!

Copy link
Contributor

@aaron-prindle aaron-prindle left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@dorbin dorbin left a comment

Choose a reason for hiding this comment

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

Looks good, Maggie! Thanks!

@aaron-prindle aaron-prindle merged commit f0eb238 into GoogleContainerTools:main May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Docs) Update Verify doc and skaffold.yaml reference with properties under executionMode.kubernetesCluster
3 participants