Support for `SOURCE_DATE_EPOCH` in `tkn bundle push` #2137

zregvart · 2023-09-28T11:57:08Z

Changes

First commit is a refactoring commit to move the code from PreRunE to parseArgsAndFlags, this is something that in full honesty should have been done on #2133. This way is easier to test the implementation.

Second commit introduces support for SOURCE_TIME_EPOCH environment variable in tkn bundle push.

Fixes #2136

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Run the code checkers with make check
Regenerate the manpages, docs and go formatting with make generated
Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

Created time in OCI Image Configuration in `tkn bundle push` can be provided using the SOURCE_TIME_EPOCH environment variable

This refactor should make it easier to test setting ctime.

tekton-robot · 2023-09-28T11:57:17Z

Hi @zregvart. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

chmouel · 2023-09-28T12:08:47Z

any chance you can explain why this is needed ? and what is this needed to be a flag ? is it expected to be used by a large number of users?

i am afraid having more and more flags in CLI with no clear explanation of what it does will just end up confusing and a burden for the maintainers.

zregvart · 2023-09-28T12:12:22Z

any chance you can explain why this is needed ? and what is this needed to be a flag ? is it expected to be used by a large number of users?

This is not adding another flag but allowing the existing flag (--ctime) to be specified with the SOURCE_DATE_EPOCH environment variable. This is in accordance with the specification from reproducible builds project.

i am afraid having more and more flags in CLI with no clear explanation of what it does will just end up confusing and a burden for the maintainers.

Oh, for sure, but we're not adding one here :)

chmouel · 2023-09-28T12:41:11Z

Oh, for sure, but we're not adding one here :)

ah i misread the PR sorry about that, this is fine by me if it's following the spec

/ok-to-test

chmouel · 2023-09-28T12:42:26Z

but another thing, what about making ctime the default? as discussed on another issue i don't believe there is breakage that could happen by changing the behaviour...

zregvart · 2023-09-28T14:16:29Z

but another thing, what about making ctime the default? as discussed on another issue i don't believe there is breakage that could happen by changing the behaviour...

Yeah, I plan on doing that, chose this to do first because of the refactoring that I did here. The followup in #2135 should be just a couple of lines following this PR :)

zregvart · 2023-09-28T14:18:30Z

I see the tests fail, I'm going to investigate a bit, most likely something to do with the time zones...

zregvart · 2023-09-29T12:53:51Z

Took me a minute to realize that the test runner is setting SOURCE_DATE_EPOCH, and that interfered with the tests.

When the `SOURCE_DATE_EPOCH` environment variable is set and the `--ctime` parameter is not provided, the unix timestamp from `SOURCE_DATE_EPOCH` will be used for setting the created time of the OCI Tekton bundle image.

zregvart · 2023-10-02T08:40:13Z

/retest-required

chmouel · 2023-10-03T17:34:48Z

pkg/cmd/bundle/push_test.go

+func init() {
+	// fixed time to ease testing
+	now = func() time.Time {
+		return time.Date(2023, 9, 22, 1, 2, 3, 0, time.UTC)


we usually use fakeclock/clockwork for that

zregvart · 2023-10-04T09:13:29Z

/retest-required

chmouel · 2023-10-04T18:57:07Z

/lgtm

Thanks!

chmouel · 2023-10-04T18:57:57Z

pkg/cmd/bundle/push.go

@@ -22,6 +23,7 @@ import (
 	"time"

 	"github.com/google/go-containerregistry/pkg/name"
+	"github.com/jonboulle/clockwork"


thanks addressing other parts of the code

vdemeester · 2023-10-05T15:29:55Z

/approve

tekton-robot · 2023-10-05T15:30:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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:

~~OWNERS~~ [vdemeester]

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

Move ctime parsing from RunE to parseArgsAndFlags

878f4c5

This refactor should make it easier to test setting ctime.

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 28, 2023

tekton-robot requested review from chmouel and vinamra28 September 28, 2023 11:57

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 28, 2023

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 28, 2023

zregvart changed the title ~~Support for SOURCE_TIME_EPOCH in tkn bundle push~~ Support for SOURCE_DATE_EPOCH in tkn bundle push Sep 28, 2023

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 28, 2023

zregvart force-pushed the issue/2136 branch from 5851a0e to 5790462 Compare September 29, 2023 12:53

Add support for SOURCE_DATE_EPOCH

ba66b37

When the `SOURCE_DATE_EPOCH` environment variable is set and the `--ctime` parameter is not provided, the unix timestamp from `SOURCE_DATE_EPOCH` will be used for setting the created time of the OCI Tekton bundle image.

zregvart force-pushed the issue/2136 branch from 5790462 to ba66b37 Compare September 29, 2023 13:16

chmouel reviewed Oct 3, 2023

View reviewed changes

Use github.com/jonboulle/clockwork in tests

bb835e8

zregvart force-pushed the issue/2136 branch from 8c5ec48 to bb835e8 Compare October 4, 2023 09:14

tekton-robot assigned chmouel Oct 4, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2023

chmouel reviewed Oct 4, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2023

tekton-robot merged commit 1c52522 into tektoncd:main Oct 5, 2023
8 checks passed

zregvart deleted the issue/2136 branch October 6, 2023 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for `SOURCE_DATE_EPOCH` in `tkn bundle push` #2137

Support for `SOURCE_DATE_EPOCH` in `tkn bundle push` #2137

zregvart commented Sep 28, 2023

tekton-robot commented Sep 28, 2023

chmouel commented Sep 28, 2023

zregvart commented Sep 28, 2023

chmouel commented Sep 28, 2023

chmouel commented Sep 28, 2023

zregvart commented Sep 28, 2023

zregvart commented Sep 28, 2023

zregvart commented Sep 29, 2023

zregvart commented Oct 2, 2023

chmouel Oct 3, 2023

zregvart commented Oct 4, 2023

chmouel commented Oct 4, 2023

chmouel Oct 4, 2023 •

edited

vdemeester commented Oct 5, 2023

tekton-robot commented Oct 5, 2023

Support for SOURCE_DATE_EPOCH in tkn bundle push #2137

Support for SOURCE_DATE_EPOCH in tkn bundle push #2137

Conversation

zregvart commented Sep 28, 2023

Changes

Submitter Checklist

Release Notes

tekton-robot commented Sep 28, 2023

chmouel commented Sep 28, 2023

zregvart commented Sep 28, 2023

chmouel commented Sep 28, 2023

chmouel commented Sep 28, 2023

zregvart commented Sep 28, 2023

zregvart commented Sep 28, 2023

zregvart commented Sep 29, 2023

zregvart commented Oct 2, 2023

chmouel Oct 3, 2023

Choose a reason for hiding this comment

zregvart commented Oct 4, 2023

chmouel commented Oct 4, 2023

chmouel Oct 4, 2023 • edited

Choose a reason for hiding this comment

vdemeester commented Oct 5, 2023

tekton-robot commented Oct 5, 2023

Support for `SOURCE_DATE_EPOCH` in `tkn bundle push` #2137

Support for `SOURCE_DATE_EPOCH` in `tkn bundle push` #2137

chmouel Oct 4, 2023 •

edited