-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
docs: add overrides
and jobManifestPath
to verify docs
#8762
Conversation
@dorbin PTAL - thanks! |
@dorbin and @aaron-prindle #8743 also requests that |
Codecov Report
@@ 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
... 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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- Changed wording on line 28 to "There are two ways to optionally customize the Skaffold-generated Kubernetes Job"
- Added "optional" to describe both override configuration options on lines 37 and 38
- Commented out the optional lines in the YAML and added "[optional]" above to further clarify.
Let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this 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!
Fixes: #8743
Description
First pass at improving
skaffold verify
documentation:overrides
andjobManifestPath
options.