if git is not installed skaffold silently falls back to sha256 tagPolicy

GoogleContainerTools / skaffold

Easy and Repeatable Kubernetes Development

https://skaffold.dev/

Apache License 2.0

15.03k stars 1.62k forks source link

if git is not installed skaffold silently falls back to sha256 tagPolicy #5439

Open sev3ryn opened 3 years ago

sev3ryn commented 3 years ago

When git is not installed skaffold silently falls back to sha256 tagPolicy ignoring gitCommit tagPolicy. I've got hard time debugging why skaffold works as expected locally but wrong in CI, while versions are same

Expected behavior

FAILURE: git not installed. Can't use specified tagPolicy

Actual behavior

Silent fallback to sha256 tagPolicy. Images tagged with IMAGE_TAG is set to latest

Information

Skaffold version: 1.20.0
Operating system: all
Contents of skaffold.yaml:

apiVersion: skaffold/v2beta9
kind: Config
metadata:
  name: best
build:
  artifacts:
  - image: registry.dummy/best
    context: ./
    docker:
      dockerfile: Dockerfile
  tagPolicy:
    gitCommit:
      variant: AbbrevCommitSha
  local:
    push: true
    useBuildkit: true
deploy:
  helm:
    releases:
      - name: best
         chartPath: helm/best
         values:
            image: registry.dummy/best

Steps to reproduce the behavior

uninstall git
skaffold render
no errors present, image tag set to latest

tejal29 commented 3 years ago

Thanks @sev3ryn. team will try to reproduce this.

tejal29 commented 3 years ago

Thanks @sev3ryn. skaffold will attempt to fallback to sha256 by running the git command and looks like that is where skaffold fails. Looks like you will add to fallback here https://github.com/GoogleContainerTools/skaffold/blob/834d5eaba317b24195f2cc09dc3a850890d1a1bc/pkg/skaffold/tag/tag.go#L38

Would you be interested in contributing to the code here?

sev3ryn commented 3 years ago

@tejal29 vice versa - I don't want any fallback to happen. If tagging with specified policy failed - return error and exit. imho, this block should be removed

https://github.com/GoogleContainerTools/skaffold/blob/834d5eaba317b24195f2cc09dc3a850890d1a1bc/pkg/skaffold/runner/build_deploy.go#L184:L193

and instead just return an error

briandealwis commented 3 years ago

Bumping to p1: discussed with @nkubala and there are a few issues here:

if we do backoff to a different tagger, it should not be silent
we should not backoff to the sha256 tagger since it actually overwrites latest which is almost certainly not what anybody would want. The new inputDigest is the better choice since it's virtually certain to not conflict with any other tags.

tejal29 commented 3 years ago

@nkubala dropping the priority since you are not actively working on it. feel free to bump it up.

briandealwis commented 2 years ago

Note: I think it makes sense to fallback if the user hasn't specified a tag policy. But if the user specified the gitCommit tag policy and git was not resolved, then we should fail.