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

"skaffold deploy" is not useful #922

Closed
ahmetb opened this issue Aug 23, 2018 · 17 comments · Fixed by #2001
Closed

"skaffold deploy" is not useful #922

ahmetb opened this issue Aug 23, 2018 · 17 comments · Fixed by #2001
Assignees
Labels
area/deploy kind/bug Something isn't working kind/feature-request priority/p2 May take a couple of releases
Milestone

Comments

@ahmetb
Copy link
Contributor

ahmetb commented Aug 23, 2018

I cannot figure out the use case for "skaffold deploy".

This command is not able to populate the image tags. Without that, all it's doing is corrupting the Deployments by deploying the tag-less images.

This command cannot use the result of "skaffold build" like this (as far as I know):

skaffold build > images.txt
skaffold deploy < images.txt

Without these functionality this command just appears like a kubectl apply that is not compatible with skaffold run to me.

Please consider either making "deploy" cmd work with "build" cmd or consider eliminating to avoid confusing users.

@dgageot
Copy link
Contributor

dgageot commented Aug 23, 2018

I agree that this command is not working well.

@dgageot dgageot added kind/bug Something isn't working kind/feature-request labels Aug 23, 2018
@balopat
Copy link
Contributor

balopat commented Aug 23, 2018

The use case would be to use skaffold deploy as part of other orchestration tools, e.g. https://github.com/GoogleContainerTools/skaffold-tools-for-java or an IDE to call it. I think we should figure out the tagging situation with skaffold deploy.

@mxey
Copy link

mxey commented Aug 24, 2018

See also #653

@balopat balopat added the priority/p2 May take a couple of releases label Aug 27, 2018
@cliveza
Copy link

cliveza commented Aug 28, 2018

Yeah, I just picked up on this too. I thought I could `

RELEASE=v1.0.0 skaffold build
RELEASE=v1.0.0 skaffold deploy

While the build command is able to build an image with the correct tag, using an environment variable and envTemplate, the deploy command just ignore this all together.

@r2d4
Copy link
Contributor

r2d4 commented Sep 5, 2018

I think the right fix for this is that skaffold deploy should also generate the tag through the git commit tagger (or env tagger).

@r2d4
Copy link
Contributor

r2d4 commented Sep 5, 2018

Eventually we might want to get rid of the "non-stable" taggers that rely on information from the build artifact itself

@rsanders
Copy link
Contributor

Could skaffold build write the build artifact names to an output file that is also read by skaffold deploy? I see that it has support for outputting those, but it leaves the plumbing as an exercise for the user.

We are using skaffold in our CD pipeline, and we like having "non-stable" components like build ID in the docker image tag. We also prefer to have separate build -> test -> deploy stages.

@r2d4
Copy link
Contributor

r2d4 commented Sep 21, 2018

@rsanders yes, skaffold build already outputs the go template of the struct that deploy uses. Deploy should be able to read from that generated output

I imagine something like

skaffold build > build.out
# do some work
skaffold deploy --build-artifacts=build.out

or

skaffold build | skaffold deploy --build-artifacts -

or

skaffold build > build.out
# do some work
skaffold deploy --build-artifacts - < build.out

@balopat
Copy link
Contributor

balopat commented Feb 15, 2019

cc @tejal29

@FX-HAO
Copy link

FX-HAO commented Feb 21, 2019

I can't agree with this, i use build and deploy in different pipelines. Sometimes I just need to build the image and then I can decide if we should deploy it.

@tejal29
Copy link
Member

tejal29 commented Mar 20, 2019

My Implementation plan right now is

Follow-up ideas:

  • "explore inference use-cases": figure out if skaffold deploy can infer --build-artifacts from git tagger or env tagger definition found in skaffold.yaml plus maybe leveraging the artifact cache locally

@pepjo
Copy link

pepjo commented Mar 20, 2019

Is this issue linked to this one: #1352 ?

@tejal29
Copy link
Member

tejal29 commented Mar 20, 2019

So with the changes as described above this is how the workflow will look like

  1. First run skaffold build
skaffold build --quiet > build.out

( Note: untill we support git tagger or env tagger artifacts listed in build.out with be digest based)
2. Now deploy

skaffold deploy --build-artifacts build.out

Note: --build-artifacts is a new flag to skaffold deploy.

@dgageot and @balopat, i feel,
skaffold build --quiet is unhelpful from user and IDE perspective because,

  • If there was a built error, you only see the error on stdout. The Build log is discarded. Users or even IDE's will have to re-run the command without --quiet flag to see at what stage the build failed.
    e.g.
../../out/skaffold build --quiet > test_error.out
FATA[0000] introducing an error forcefully              
tejaldesai@tejaldesai~/go/src/github.com/GoogleContainerTools/skaffold/examples/getting-started: (pr/tejal29/1841)$ 

Maybe we could an explicit flag to build like:

  1. --list-built-artifacts : make it more explicit that artifacts build will be written to a file.
    If this flag is present, users can still see the build output on std in addition to build results written to file.

What do you think?
This could be an additional task i can add once we all agree.

tejal29 added a commit to tejal29/skaffold that referenced this issue Mar 22, 2019
1. Inputfilepath:
	Inputfilepath flag, makes sure the file present exists.
2. Outputfilepath:
	Outputfilepath flag does not do any validation right now.

Motivation:
For, GoogleContainerTools#922 I am going to add `--build-output` as new flag to `skaffold build` to
dump all artifacts built with with their tags.

Similarly, the deploy will have additional flag `--build-artifacts` which will
point to the output written by `skaffold build --build-output <file>`

The Inputfilepath, can also be used to replace all the places where we
expect an input file without repeating validation.

```
(add_flag_to_deploy)$ git grep 'Filename or URL to the pipeline'
cmd/skaffold/app/cmd/cmd.go:    cmd.Flags().StringVarP(&opts.ConfigurationFile, "filename", "f", "skaffold.yaml", "Filename or URL to the pipeline file")
cmd/skaffold/app/cmd/diagnose.go:       cmd.Flags().StringVarP(&opts.ConfigurationFile, "filename", "f", "skaffold.yaml", "Filename or URL to the pipeline file")
cmd/skaffold/app/cmd/fix.go:    cmd.Flags().StringVarP(&opts.ConfigurationFile, "filename", "f", "skaffold.yaml", "Filename or URL to the pipeline file")
cmd/skaffold/app/cmd/init.go:   cmd.Flags().StringVarP(&opts.ConfigurationFile, "filename", "f", "skaffold.yaml", "Filename or URL to the pipeline file")
```
tejal29 added a commit to tejal29/skaffold that referenced this issue Mar 22, 2019
1. Inputfilepath:
        Inputfilepath flag, makes sure the file present exists.
2. Outputfilepath:
        Outputfilepath flag does not do any validation right now.

Motivation:
    For, GoogleContainerTools#922 I am going to add `--build-output` as new flag to `skaffold build` to
    dump all artifacts built with with their tags.

    Similarly, the deploy will have additional flag `--build-artifacts` which will
    point to the output written by `skaffold build --build-output <file>`

    The Inputfilepath, can also be used to replace all the places where we
    expect an input file without repeating validation.

    ```
    (add_flag_to_deploy)$ git grep 'Filename or URL to the pipeline'
    cmd/skaffold/app/cmd/cmd.go:    cmd.Flags().StringVarP(&opts.ConfigurationFile, "filename", "f", "skaffold.yaml", "Filename or URL to the pipeline file")
    cmd/skaffold/app/cmd/diagnose.go:       cmd.Flags().StringVarP(&opts.ConfigurationFile, "filename", "f", "skaffold.yaml", "Filename or URL to the pipeline file")
    cmd/skaffold/app/cmd/fix.go:    cmd.Flags().StringVarP(&opts.ConfigurationFile, "filename", "f", "skaffold.yaml", "Filename or URL to the pipeline file")
    cmd/skaffold/app/cmd/init.go:   cmd.Flags().StringVarP(&opts.ConfigurationFile, "filename", "f", "skaffold.yaml", "Filename or URL to the pipeline file")
    ```
@pepjo
Copy link

pepjo commented Mar 26, 2019

I am trying to understand the need for this when trying to deploy with an image that has already been built:

skaffold build --quiet > build.out

Our current planned use case is:

  1. If someone triggers a pull request skaffold build should be triggered in CI (creating and pushing an image with the git commit as the tag)
  2. When the pull request is merged CI would run skaffold deploy to deploy the image that we already created. It will use the same naming convention to try to deploy the image that corresponds to the commit. This only works if the merges are fast-forward as the git commit reference will be kept. A useful addition would be to add a flag that checks with the docker registry if the image does indeed exist and return 1 if not.

Adding the build.out is complicated (and unnecessary?) in this use case. We do continuous deployment so it's ok for us to not have meaningful docker tags (eg. with version numbers).

Will this use case be supported with the planned implementation?

@tejal29
Copy link
Member

tejal29 commented Mar 26, 2019

Adding the build.out is complicated (and unnecessary?) in this use case. We do continuous deployment so it's ok for us to not have meaningful docker tags (eg. with version numbers).

The first thing we are going to address is deploy should be able to consume build output and its easier if we first implement a build.out based approach.

Will this use case be supported with the planned implementation?

@pepjo we mention your use case in follow up ideas where skaffold deploy can infer tags from git or an env variable.

@pepjo
Copy link

pepjo commented Mar 26, 2019

Thanks, I did not see that particular part of the comment. Thanks for the answer.

tejal29 added a commit to tejal29/skaffold that referenced this issue Apr 18, 2019
For GoogleContainerTools#922, i am making changes to `skaffold deploy` to  not run `skaffold build`.
`--images` flag is only used `deploy` flow.
The way it works is when `--images` flag is present, runner creates a
`PreBuiltImageBuilder`. All it does is, parses the images and converts
them to build.Artifact. If during deploy, an image in the list was also
built, then it uses the built image tag vs the tag present on the
commmand line flag.

In this change, we remove creating a PreBuiltImageBuilder in the runner.
(Remember this flag is only available for deploy and hence other command
won't get affected)
The `--images` will now be converted to build.Artifact in the
runner.Deploy flow.
@tejal29
Copy link
Member

tejal29 commented Apr 18, 2019

#1986 fixes one of the subtasks for "Verify #1352 still exists and if it does, do not run skaffold build in skaffold deploy"

tejal29 added a commit to tejal29/skaffold that referenced this issue Apr 19, 2019
For GoogleContainerTools#922, i am making changes to `skaffold deploy` to  not run `skaffold build`.
`--images` flag is only used `deploy` flow.
The way it works is when `--images` flag is present, runner creates a
`PreBuiltImageBuilder`. All it does is, parses the images and converts
them to build.Artifact. If during deploy, an image in the list was also
built, then it uses the built image tag vs the tag present on the
commmand line flag.

In this change, we remove creating a PreBuiltImageBuilder in the runner.
(Remember this flag is only available for deploy and hence other command
won't get affected)
The `--images` will now be converted to build.Artifact in the
runner.Deploy flow.
tejal29 added a commit to tejal29/skaffold that referenced this issue Apr 22, 2019
For GoogleContainerTools#922, i am making changes to `skaffold deploy` to  not run `skaffold build`.
`--images` flag is only used `deploy` flow.
The way it works is when `--images` flag is present, runner creates a
`PreBuiltImageBuilder`. All it does is, parses the images and converts
them to build.Artifact. If during deploy, an image in the list was also
built, then it uses the built image tag vs the tag present on the
commmand line flag.

In this change, we remove creating a PreBuiltImageBuilder in the runner.
(Remember this flag is only available for deploy and hence other command
won't get affected)
The `--images` will now be converted to build.Artifact in the
runner.Deploy flow.
tejal29 added a commit to tejal29/skaffold that referenced this issue May 10, 2019
1. Add kubectl apply command after setting labels
2. Integration tests for helms are back
   a. GoogleContainerTools#1823: Use env template in skaffold release name
3. During helm deploy, build is assumed and hence if no build-artifacts
are supplied, it fails with following error
 "no build present for gcr.io/k8s-skaffold/skaffold-helm"

  Since Build and Deploy are now separate ( GoogleContainerTools#922 for notes) use the
  image value as is if not present as build artifacts

TODO:
1. Add PR description
2. Figure out why we get 2 pods, 1 with valid labels which is running
   but another 1 with different labels.
tejal29 added a commit to tejal29/skaffold that referenced this issue May 10, 2019
1. Add kubectl apply command after setting labels
2. Integration tests for helms are back
   a. GoogleContainerTools#1823: Use env template in skaffold release name
3. During helm deploy, build is assumed and hence if no build-artifacts
are supplied, it fails with following error
 "no build present for gcr.io/k8s-skaffold/skaffold-helm"

  Since Build and Deploy are now separate ( GoogleContainerTools#922 for notes) use the
  image value as is if not present as build artifacts
  Fix test fot this.

TODO:
1. Add PR description
2. Figure out why we get 2 pods, 1 with valid labels which is running
   but another 1 with different labels.
tejal29 added a commit to tejal29/skaffold that referenced this issue May 13, 2019
1. Add kubectl apply command after setting labels
2. Integration tests for helms are back
   a. GoogleContainerTools#1823: Use env template in skaffold release name
3. During helm deploy, build is assumed and hence if no build-artifacts
are supplied, it fails with following error
 "no build present for gcr.io/k8s-skaffold/skaffold-helm"

  Since Build and Deploy are now separate ( GoogleContainerTools#922 for notes) use the
  image value as is if not present as build artifacts
  Fix test fot this.

TODO:
1. Add PR description
2. Figure out why we get 2 pods, 1 with valid labels which is running
   but another 1 with different labels.
tejal29 added a commit to tejal29/skaffold that referenced this issue May 14, 2019
On master, skaffold deploy fails with following error.

```
skaffold deploy -f examples/helm-deployment/skaffold.yaml
Starting deploy...
Error: release: "skaffold-helm" not found
Helm release skaffold-helm not installed. Installing...
FATA[0002] deploying skaffold-helm: matching build results to chart values: no build present for gcr.io/k8s-skaffold/skaffold-helm
```
This is because in GoogleContainerTools#922, we removed deploy triggering the build. Running
deploy should use the default tag i.e. "latest" when depoying images.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/deploy kind/bug Something isn't working kind/feature-request priority/p2 May take a couple of releases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants