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

Add support for additional tags #802

Merged
merged 5 commits into from
Sep 27, 2021

Conversation

sambhav
Copy link
Contributor

@sambhav sambhav commented Aug 11, 2021

Adds support for additional tags to be specified as a part of the image spec. This would allow users to manually patch and specify additional tags to be pushed out. This can allow kpack users to push semver tags or other tags like git commit shas by a utility like kp cli.

Example use cases -

kp image save --additional-tag myimage:v1.2 --additional-tag myimage:$(git rev-parse HEAD)

where the above would override the additionalTags array entirely with the newly supplied values

to patch and tag images with commit shas or version numbers.

Fixes #802 and possibly #755

@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2021

Codecov Report

Merging #802 (6d6ba7a) into main (747d2fd) will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #802      +/-   ##
==========================================
+ Coverage   67.80%   67.92%   +0.11%     
==========================================
  Files         112      112              
  Lines        4846     4863      +17     
==========================================
+ Hits         3286     3303      +17     
  Misses       1191     1191              
  Partials      369      369              
Impacted Files Coverage Δ
pkg/apis/build/v1alpha2/image_types.go 50.00% <ø> (ø)
pkg/apis/build/v1alpha1/build_validation.go 78.12% <100.00%> (ø)
pkg/apis/build/v1alpha2/build_validation.go 81.08% <100.00%> (ø)
pkg/apis/build/v1alpha2/image_builds.go 61.83% <100.00%> (+0.59%) ⬆️
pkg/apis/build/v1alpha2/image_validation.go 100.00% <100.00%> (ø)

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 747d2fd...6d6ba7a. Read the comment docs.

@@ -5,7 +5,8 @@ kpack will monitor the inputs to the image configuration to rebuild the image wh

The following defines the relevant fields of the `image` resource spec in more detail:

- `tag`: The image tag.
- `tag`: The image tag. This is the primary image tag that is immutable for a given image and is re-used during subsequent builds for certain cached data.
- `additionalTags`: Any additional list of image tags that should be published. This list of tags is mutable.
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 am a bit conflicted if we should only allow additional tags to be tag names or fully fledged docker image tags. The former would be easier in terms of secret management etc. and ensuring that pushes succeed where as the latter would be more flexible. I am tempted to switch to the former but I have currently implemented the latter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The docker terminology around tags makes this quite cumbersome. We really want to just provide a mechanism to add additional tags to the same repo but, the kpack api has used the term tag to reference the entire image name (similar to --tag on docker build). It would be ideal if we had a terminology to provide just a list of simple tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we call it additionalRefs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe? Ref sounds like it might be referring to another kubernetes resource.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems relevant: buildpacks/community#128

@sambhav
Copy link
Contributor Author

sambhav commented Aug 25, 2021

@matthewmcnew ping on reviews for this PR. This would be really helpful for our use cases.

@@ -89,6 +90,10 @@ func (is *ImageSpec) validateTag(ctx context.Context) *apis.FieldError {
return validate.Tag(is.Tag)
}

func (is *ImageSpec) validateAdditionalTag(ctx context.Context) *apis.FieldError {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we validate that the tags are valid "tags" in the same registry similar to the lifecycle?

@matthewmcnew
Copy link
Collaborator

I was originally a bit reluctant to allow kpack Images to have mutable tags because I feared it would encourage the referencing images over digests. I think I have come around to it. My only hesitation is the complication in modeling it in the api. (eg. including .spec.tags on Builds and .spec.tag & .spec.additionalTags on Images)

Signed-off-by: GitHub <noreply@github.com>
@sambhav
Copy link
Contributor Author

sambhav commented Sep 16, 2021

@matthewmcnew this is ready for a review

@sambhav
Copy link
Contributor Author

sambhav commented Sep 24, 2021

@matthewmcnew can we merge this?

@matthewmcnew
Copy link
Collaborator

@samj1912 We plan on merging today.

Thank you for your patience :)

@tomkennedy513 tomkennedy513 merged commit 3768e62 into buildpacks-community:main Sep 27, 2021
@sambhav sambhav deleted the additional-tags branch September 27, 2021 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants