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.

Sign up for GitHub

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

Jump to bottom

Expand the skaffold init --artifact API to allow specifying artifact context #5000

Merged
merged 5 commits into from Nov 11, 2020
Merged

Expand the skaffold init --artifact API to allow specifying artifact context #5000

merged 5 commits into from Nov 11, 2020

Conversation

MarlonGamez
Copy link
Contributor

Fixes #3814

Description
This PR allow for users to specify a context when passing artifacts into skaffold init --artifact.

Previously, users could specify the fields "builder", "payload", and "image", but can now additionally specify "context". Omitting the "context" field will result in skaffold's typical context suggestions.

For example:

skaffold init \
  -a '{"builder":"Docker","payload":{"path":"leeroy-app/Dockerfile"},"image":"gcr.io/k8s-skaffold/leeroy-app"}' \
  -a '{"builder":"Docker","payload":{"path":"leeroy-web/Dockerfile"},"image":"gcr.io/k8s-skaffold/leeroy-web","context":"path/to/src"}'

would result in the following config

apiVersion: skaffold/v2beta9
kind: Config
metadata:
  name: microservices
build:
  artifacts:
  - image: gcr.io/k8s-skaffold/leeroy-app
    context: leeroy-app
    docker:
      dockerfile: Dockerfile
  - image: gcr.io/k8s-skaffold/leeroy-web
    context: path/to/src
    docker:
      dockerfile: Dockerfile
deploy:
  kubectl:
    manifests:
    - leeroy-app/kubernetes/deployment.yaml
    - leeroy-web/kubernetes/deployment.yaml

@google-cla google-cla bot added the cla: yes label Nov 5, 2020
@codecov
Copy link

codecov bot commented Nov 5, 2020 •
edited

Codecov Report

Merging #5000 (d51c76f) into master (6c9376c) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5000      +/-   ##
==========================================
- Coverage   72.16%   72.11%   -0.05%     
==========================================
  Files         363      365       +2     
  Lines       12762    12776      +14     
==========================================
+ Hits         9210     9214       +4     
- Misses       2866     2877      +11     
+ Partials      686      685       -1
Impacted Files Coverage Δ
pkg/skaffold/initializer/build/builders.go 70.00% <ø> (ø)
pkg/skaffold/initializer/build/cli.go 75.86% <100.00%> (+0.42%) ⬆️
pkg/skaffold/initializer/build/util.go 100.00% <100.00%> (ø)
pkg/skaffold/build/dependencies.go 33.33% <0.00%> (-19.30%) ⬇️
pkg/skaffold/build/cluster/kaniko.go 22.36% <0.00%> (-5.41%) ⬇️
pkg/skaffold/build/local/prune.go 70.11% <0.00%> (-3.38%) ⬇️
pkg/skaffold/docker/image.go 79.34% <0.00%> (-1.28%) ⬇️
pkg/skaffold/build/cluster/cluster.go 25.80% <0.00%> (-0.87%) ⬇️
pkg/skaffold/build/cluster/pod.go 86.56% <0.00%> (-0.39%) ⬇️
pkg/skaffold/deploy/helm/helm.go 73.80% <0.00%> (-0.31%) ⬇️
... and 10 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 6c9376c...d51c76f. Read the comment docs.

@MarlonGamez MarlonGamez marked this pull request as ready for review November 9, 2020 18:38
@MarlonGamez MarlonGamez requested a review from a team as a code owner November 9, 2020 18:38
@MarlonGamez MarlonGamez changed the title Expands the skaffold init --artifact API to allow specificying artifact context Expand the skaffold init --artifact API to allow specifying artifact context Nov 9, 2020
briandealwis
briandealwis reviewed Nov 10, 2020
View reviewed changes
docs/content/en/docs/pipeline-stages/init.md Outdated Show resolved Hide resolved
pkg/skaffold/initializer/build/cli.go Outdated Show resolved Hide resolved
pkg/skaffold/initializer/build/cli.go Outdated
return nil
}

func processCliArtifacts(cliArtifacts []string) ([]BuilderImagePair, error) {
func processCliArtifacts(cliArtifacts []string) ([]BuilderImagePair, []string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to not include the workspace in the BuilderImagePair?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to do this too. The current way that I did things make it more bloated. I'll work on this change.

@MarlonGamez
Copy link
Contributor Author

@briandealwis fixed up some things based off your suggestions. PTAL when you get the chance :)

briandealwis
briandealwis approved these changes Nov 11, 2020
View reviewed changes
pkg/skaffold/initializer/build/builders.go
@@ -51,6 +51,7 @@ type InitBuilder interface {
type BuilderImagePair struct {
Copy link
Member

Choose a reason for hiding this comment

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

The name is a bit funny. I wonder if it's worth renaming now as something like ProtoArtifact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, was thinking of renaming it to something like ArtifactInfo or something similar like what you suggested. I'll make a follow up PR to rename it. Didn't want to do it here as there are quite a few instances and didn't want to bloat things more

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

@MarlonGamez MarlonGamez merged commit eef156f into GoogleContainerTools:master Nov 11, 2020
@MarlonGamez MarlonGamez mentioned this pull request Nov 12, 2020
Merged
@fmotrifork fmotrifork mentioned this pull request Nov 25, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@briandealwis briandealwis briandealwis approved these changes

@nkubala nkubala Awaiting requested review from nkubala

Assignees
No one assigned
Labels
cla: yes size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FR] support setting artifact.context in skaffold init
2 participants
@MarlonGamez @briandealwis