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 commit SHA label to SHP image #776

Merged

Conversation

qu1queee
Copy link
Contributor

@qu1queee qu1queee commented May 11, 2021

Changes

Adding the SHA that trigger the build of https://quay.io/repository/shipwright/shipwright-operator as a label. This is mainly to have a clear mapping to the commit on this same repository.

Note: Wondering if we should use quay.expires-after label, for the nightly images, we can set a max lifetime for those ones, so that we keep pruning them.

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Add SHA label to SHP controller img

@qu1queee qu1queee added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 11, 2021
@openshift-ci openshift-ci bot added release-note Label for when a PR has specified a release note and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 11, 2021
@openshift-ci openshift-ci bot requested review from imjasonh and mattcui May 11, 2021 06:38
@qu1queee qu1queee removed the request for review from mattcui May 11, 2021 06:38
@qu1queee qu1queee changed the title Add commit SHA label to SHP image WIP: Add commit SHA label to SHP image May 11, 2021
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2021
Copy link
Contributor

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

Two questions:

  • Should we tag the image instead of (or in addition to) labeling it?
  • Should we annotate the image instead of labeling? There's a OCI standard annotation for the revision.

Makefile Outdated Show resolved Hide resolved
@qu1queee qu1queee force-pushed the qu1queee/uselabels_img branch 4 times, most recently from 5d4d051 to 97535bb Compare May 12, 2021 15:38
@qu1queee
Copy link
Contributor Author

@imjasonh so to answer your qs

Should we tag the image instead of (or in addition to) labeling it?

We tag them today already right?

Should we annotate the image instead of labeling? There's a OCI standard annotation for the revision.

This was interesting. I realized ko doesn´t support annotations today, so I will say that we cannot do it now via ko. I read image-spec/annotations and I assume your suggestion on annotation is due to them superseding labels. Because we cannot use annotations with ko I updated my label key to follow a combination of reverse domain notation + compatible prefix . Leading to io.shipwright.vcs-ref, see the description of the vcs-ref label in label-schema.org.

In a nutshell, this PR will now add the label to images build via the nightly or once we ship a new release.

@qu1queee qu1queee changed the title WIP: Add commit SHA label to SHP image Add commit SHA label to SHP image May 12, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 12, 2021
@qu1queee qu1queee requested a review from imjasonh May 12, 2021 15:47
@imjasonh
Copy link
Contributor

Should we tag the image instead of (or in addition to) labeling it?

We tag them today already right?

Nope, we tag them as v0.4.0 and v0.4.0-debug, or nightly-2021-05-12-1620796767(-debug), but not with the commit SHA.

https://quay.io/repository/shipwright/shipwright-operator

label-schema.org labels sgtm, when ko supports annotations we should annotate the manifest too, but it's not a big deal.

@qu1queee
Copy link
Contributor Author

Nope, we tag them as v0.4.0 and v0.4.0-debug, or nightly-2021-05-12-1620796767(-debug), but not with the commit SHA.

@imjasonh I see. In general I didnt want to interfere with the semantic versioning we have in place. If doing something like a docker inspect on an image generated by the nightly or the release CI would give me a reference to the exact commit SHA from where it was generated, then my goal was achieved. The value of adding a tag with the commit SHA is that the ref to source code is clear, and you do not need to inspect the image (from what I can see today), but I would like to avoid more images on top of what we have. Whats your thinking?

@imjasonh
Copy link
Contributor

@imjasonh I see. In general I didnt want to interfere with the semantic versioning we have in place. If doing something like a docker inspect on an image generated by the nightly or the release CI would give me a reference to the exact commit SHA from where it was generated, then my goal was achieved. The value of adding a tag with the commit SHA is that the ref to source code is clear, and you do not need to inspect the image (from what I can see today), but I would like to avoid more images on top of what we have. Whats your thinking?

I don't have a strong preference, and this is obviously an improvement over what we have today, so I think we should do it.

The benefit of tagging would be in aiding debugging an issue at a given range of commits, but that's probably a small enough benefit that it's reasonable that it might not be worth the added complexity.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2021
Copy link
Member

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

/approve

Fine using our own namespace, since we plan on using annotations once ko supports it.

Comment on lines +18 to +19
KO_DOCKER_REPO="$IMAGE_HOST/$IMAGE" GOFLAGS="${GO_FLAGS}" ko resolve -t "$TAG" --image-label "io.shipwright.vcs-ref=${GITHUB_SHA}" --bare --platform=all -R -f deploy/ > release.yaml
KO_DOCKER_REPO="$IMAGE_HOST/$IMAGE" GOFLAGS="${GO_FLAGS} -tags=pprof_enabled" ko resolve -t "$TAG-debug" --image-label "io.shipwright.vcs-ref=${GITHUB_SHA}" --bare --platform=all -R -f deploy/ > release-debug.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Just making a note that we are using a label-schema inspired label, and not label-schema itself. To do so would require using the org.label-schema namespace and also supplying the schema-version label.

See https://label-schema.org/rc1/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adambkaplan good catch, indeed this is a label-schema inspired, not fully compliant with the docs there.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 14, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 14, 2021
@openshift-merge-robot openshift-merge-robot merged commit e356a3d into shipwright-io:master May 14, 2021
@qu1queee qu1queee deleted the qu1queee/uselabels_img branch May 17, 2021 08:08
@adambkaplan adambkaplan added this to the release-v0.5.0 milestone Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Label for when a PR has specified a release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants