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

Extending Workflow for Kpt Deployer (accepting additional arguments) #4736

Merged
merged 4 commits into from
Aug 28, 2020
Merged

Extending Workflow for Kpt Deployer (accepting additional arguments) #4736

merged 4 commits into from
Aug 28, 2020

Conversation

felixtran39
Copy link
Contributor

Related: #3904

Description
This contains the implementation and additional test cases allowing users to specify additional arguments when running kpt fn or kpt live commands with Skaffold.

User facing changes
Users will have the option to specify additional arguments in their skaffold.yaml.

Follow-up Work
It may be a good idea to add more validation to the config in the future such as validating the values of these arguments to fit certain patterns and catch the error before the command is called.

Before:

deploy:
  kpt:
    dir: ...
    fn:
      fnPath: ...
      image: ...
    live:
      apply:
      - ...
      - ...

After:

deploy:
  kpt:
    dir: ...
    fn:
      fnPath: str
      image: str
      network: str
      networkName: str
      globalScope: str
      mount: []str
    live:
      inventoryID: str
      inventoryNamespace: str
      apply:
        pollperiod: str
        prunePropagationPolicy: str
        pruneTimeout: str
        reconcileTimeout: str
      

pkg/skaffold/deploy/kpt.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/kpt.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 28, 2020

Codecov Report

Merging #4736 into master will decrease coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4736      +/-   ##
==========================================
- Coverage   73.45%   73.38%   -0.08%     
==========================================
  Files         343      345       +2     
  Lines       13585    13739     +154     
==========================================
+ Hits         9979    10082     +103     
- Misses       2982     3023      +41     
- Partials      624      634      +10     
Impacted Files Coverage Δ
pkg/skaffold/schema/latest/config.go 100.00% <ø> (ø)
pkg/skaffold/deploy/kpt.go 86.84% <100.00%> (+2.56%) ⬆️
pkg/skaffold/kubernetes/context/context.go 71.42% <0.00%> (-13.94%) ⬇️
pkg/skaffold/deploy/status_check.go 52.25% <0.00%> (-6.81%) ⬇️
pkg/skaffold/util/util.go 86.07% <0.00%> (-0.86%) ⬇️
pkg/diag/validator/pod.go 1.60% <0.00%> (-0.08%) ⬇️
pkg/skaffold/config/util.go 69.31% <0.00%> (ø)
pkg/skaffold/deploy/resource/logfile.go 84.61% <0.00%> (ø)
pkg/skaffold/cluster/minikube.go 68.75% <0.00%> (ø)
pkg/skaffold/deploy/resource/deployment.go 91.54% <0.00%> (+1.54%) ⬆️
... and 2 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 abdbe17...56ec0cc. Read the comment docs.

Copy link
Contributor

@yuwenma yuwenma left a comment

Choose a reason for hiding this comment

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

Some minor comments

Copy link
Contributor

@yuwenma yuwenma left a comment

Choose a reason for hiding this comment

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

Sweet! Great job on the last PR

Copy link
Contributor

@MarlonGamez MarlonGamez left a comment

Choose a reason for hiding this comment

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

LGTM!

@MarlonGamez MarlonGamez merged commit 8d8379d into GoogleContainerTools:master Aug 28, 2020
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.

4 participants