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

Port the auto tag feature from docker plugin #36

Merged
merged 3 commits into from Feb 10, 2022
Merged

Conversation

15cm
Copy link
Contributor

@15cm 15cm commented Jan 20, 2022

Support the feature in the "Autotag" section of docker plugin.

The logic is forked from https://github.com/drone-plugins/drone-docker code base with necessary modification. I've tested it e2e for DockerHub on my Drone server via this plugin image https://hub.docker.com/repository/docker/15cm/drone-kaniko, for both tag pushes and commit pushes.

With this change the .drone.yml in this repo should work as intended.

Other changes:

  • Rename the existing "auto tag" flags/code to "expand tag" for a less misleading naming.
  • ATTENTION: make a breaking change to set default value of "--tags" to empty.
    Rationale is to expect most users to use the auto tagging feature. When power users want to specify tags, they should always explicitly set tags instead of being surprised by the default "latest" tag.

…/drone-docker

The logic is forked from https://github.com/drone-plugins/drone-docker code base
with necessary modification. I've tested it e2e for DockerHub on my Drone server
via this plugin image https://hub.docker.com/repository/docker/15cm/drone-kaniko,
for both tag pushes and commit pushes.

With this change the .drone.yml in this repo should work as intended.

Other changes:
- Rename the existing "auto tag" flags/code to "expand tag" for a less
misleading naming.
- ATTENTION: make a breaking change to set default value of "--tags" to empty.
Rationale is to expect most users to use the auto tagging feature. When power
users want to specify tags, they should always explicitly set tags instead of
being surprised by the default "latest" tag.
@shubham149
Copy link
Contributor

We cannot make any breaking changes until and unless they are security vulnerabilities. Auto tag was already supported in this plugin. Can you update what functionality was earlier missing for auto-tags?

@15cm
Copy link
Contributor Author

15cm commented Feb 8, 2022

The existing auto tag feature behaves differently from the auto tag feature of the drone docker plugin. Comparison:

  • The current auto tag requires users to specify the tag in the CLI argument. It expands the tag into several tags following semantic versioning.
  • The new auto tag in this PR automatically detect the tag without a user provided tag. Then expand it following semantic versioning.

The reason I make breaking change on the AUTO_TAG flag instead of creating a new flag is that, the current auto tag is not "auto". I have to specify the tag, which means an update of .drome.yml per tag push. Moreover, as a user who comes from the drone docker plugin (I believe I'm not the only one) that supports the auto tag feature that behaves the same as this PR, I was surprised by the behavior of the current auto tag feature and struggled to see if it could auto detect the tag of git commits.

In summary, naming the feature in this PR as AUTO_TAG:

  • aligns the behavior with the drone docker plugin. So we don't surprise users who come from there.
  • replaces the misleading naming of the current feature that still requires a user-provided tag.

Copy link
Contributor

@shubham149 shubham149 left a comment

Choose a reason for hiding this comment

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

@15cm Sounds good. I have added a comment to the PR.

cmd/kaniko-gcr/main.go Outdated Show resolved Hide resolved
The --auto-tag has to be a breaking change. This commit limit the breaking
impact to the users who enable the flag. Behaviors of flag combination after
this commit:
* --auto-tag=false: No changes.
* --auto-tag=false,--expand-tag=true,tags=1.0.0:
  * Old behavior: Should not happen. --expand-tag didn't exist.
  * New Behavior: Build with [1,1.0,1.0.0] tags.
* --auto-tag=true
  * Old behavior: Build with the "latest" tag.
  * New behavior: Build with auto detected tags. Abort if auto detection failed.
* --auto-tag=true,tags=latest: same as "--auto-tag=true".
* --auto-tag=true,tags=1.0.0:
  * Old behavior: Build with [1,1.0,1.0.0] tags.
  * New behavior: Abort the build with an error message.
* --auto-tag=true,--expand-tag=true,tags=1.0.0: Abort the build with an error message.

Also added a test for the integration of the BUILD struct and the tagger
package, which is used by kaniko.go.
@shubham149 shubham149 merged commit de43f3a into drone:main Feb 10, 2022
@shubham149
Copy link
Contributor

Thanks a lot @15cm for your contribution.

@15cm
Copy link
Contributor Author

15cm commented Feb 10, 2022

Thanks for taking care of this PR @shubham149. Sorry a link in the README added by this PR is malformatted. I created another PR as a minor fix #38.

@shubham149
Copy link
Contributor

No worries. I have merged that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants