Add revision label to function images and show in describe#3629
Conversation
Detect the git commit SHA of the function source directory using go-git and add it as a "commit" OCI image label. The short SHA is suffixed with "-dirty" if the working tree has uncommitted changes. If the function is not in a git repo, the label is omitted. Integrated into all three builders (pack, s2i, oci) and the func-util scaffold/s2i-generate pipeline.
Add ImageCommit() to read the commit label from a deployed function image. Integrate into all three describers (knative, k8s, keda) and expose via the Instance struct so it appears in JSON/YAML output of func describe.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3629 +/- ##
==========================================
+ Coverage 56.33% 56.40% +0.06%
==========================================
Files 180 181 +1
Lines 20571 20669 +98
==========================================
+ Hits 11589 11658 +69
- Misses 7780 7797 +17
- Partials 1202 1214 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/cc @lkingland @gauron99 |
|
|
||
| const ( | ||
| MiddlewareVersionLabelKey = "middleware-version" | ||
| CommitLabelKey = "commit" |
There was a problem hiding this comment.
I would name this revision to be more VCS agnostic. Or even org.opencontainers.image.revision.
There was a problem hiding this comment.
Good point. Let me update it
There was a problem hiding this comment.
I updated the label key to org.opencontainers.image.revision and the key for the "func describe" output to revision
| TlsVerify string | ||
|
|
||
| // Git commit SHA of the function source | ||
| Commit string |
There was a problem hiding this comment.
Why we need to parameterize? Cannot the task read current commit on its own?
There was a problem hiding this comment.
I think we upload source as is including .git so the task could read it.
There was a problem hiding this comment.
I had it initially in the task itself, but I wasn't able to get the commit hash there. This would also have involved to write it to a file and then read it during the prepare step. So I thought it'd be cleaner to have it simply as a parameter for the task.
lkingland
left a comment
There was a problem hiding this comment.
/lgtm
/hold for the good suggestions from others
| "github.com/google/go-containerregistry/pkg/v1/remote" | ||
| ) | ||
|
|
||
| type ImageInspector struct { |
There was a problem hiding this comment.
Removed the struct as part of the refactoring
| transport http.RoundTripper | ||
| } | ||
|
|
||
| func NewImageInspector(transport http.RoundTripper) *ImageInspector { |
There was a problem hiding this comment.
This is just a nit, but I'm not seeing any reason this needs to be a pointer, and nils as args begs the question.
Is that just to enable the nil in method signatures, like NewDeployer(x, nil)? This could be accomplished with an fn.ImageInspector interface (which is perhaps a good idea for other reasons too). The simplest option to get rid of the ponters/nils would then be to have a singleton fn.DefaultImageInspector which is a noop and can be passed in place of the nil.
There was a problem hiding this comment.
The struct got removed during the refactoring. I am using the http.transport directly now (passed via our Options pattern)
| c = newCredentialsProvider(config.Dir(), t, "") // for accessing registries | ||
| d = newKnativeDeployer(cfg.Verbose) // default deployer (can be overridden via options) | ||
| pp = newTektonPipelinesProvider(c, cfg.Verbose) | ||
| ii = fn.NewImageInspector(t) |
There was a problem hiding this comment.
Is the transport the reason for having the ImageInspector exposed outside the core into the cli (or anyone using the core api)? If so, it might be possible to just pass the transport to the describers/pipeline provider. Then they can use that internally to inspect using a helper method.
There was a problem hiding this comment.
Good point. I updated it. I am passing the http.Transport now to the describers only (via a WithDescriberTransport(t) option func). I hope this makes sense :)
Replace ImageInspector struct with ImageLabels helper in function_labels.go. Describers now accept transport via WithDescriberTransport option, keeping the transport internal and removing nil arguments from callers.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: creydr, lkingland, matejvasek The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
Changes
org.opencontainers.image.revisionimage label containing the short git commit SHA of the function source directory at build time, with a-dirtysuffix when the working tree has uncommitted changesrevisionlabel infunc describeoutput (JSON/YAML)/kind enhancement
Release Note