-
Notifications
You must be signed in to change notification settings - Fork 133
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
feat: update chain's controllers to use v1 Tekton APIs natively while converting to v1beta1 to keep formats backwards compatible #1016
Conversation
/assign @chitrangpatel |
e752b01
to
0b236db
Compare
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this massive effort 🙏 .
Left some comments.
Just to confirm my high level significant changes:
Controller:
only looks at v1 now. This means that it will receive v1 objects.
Signing, Storers etc.
Understands v1 (no longer v1beta1). Wherever there was a use of pipeline resources, we convert to v1beta1 and call the v1beta1 logic.
In formats:
- v1: we convert to v1beta1 and analyze
- v2alpha1: we convert to v1beta1
- v2alpha2: convert to v1beta1
- v2alpha3: no need for conversion. Will look at v1, which is what the controller receives.
837fff6
to
a74698d
Compare
The following is the coverage report on the affected files.
|
/approve |
ade698a
to
9579748
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
3ed1545
to
4d39859
Compare
The following is the coverage report on the affected files.
|
4d39859
to
aee1656
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
6d04a41
to
3abc21b
Compare
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! Some final questions, otherwise should be able to get this merged.
… converting to v1beta1 to keep formats backwards compatible
3abc21b
to
16b8a54
Compare
Thanks for the reviews @wlynch and @lcarva! @wlynch I believe in the final review the only remaining work item was to update the godoc for
With this change I believe this should be mergable IIUC. Thanks! |
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/kind misc |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chitrangpatel, JeromeJu, wlynch 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 |
… converting to v1beta1 to keep formats backwards compatible (tektoncd#1016)
fixes #665 , fixes #985, fixes #986
This PR updates chains to natively use the Tekton Pipeline
v1
API ("github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"), updated from the Tekton Pipelinev1beta1
API ("github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"). Internally in the provenance and signing logic v1 Tekton object representation is used w/ v1beta1 objects being converted in/out of v1 as needed for processing fields and storing provenance (to keep it as similar as possible for users upgradingchains
)The backwards compatibility works as v1beta1 and v1 Tekton APIs can be converted between types. Due to v1beta1 changes over time, some fields have their own bespoke support (v1beta1 TaskRun
.Spec.Resources
, and.Status.TaskRunStatusField.TaskSpec.Resources
).For a V1 object using the V1 controller, the flow is straightforward like it was initially with V1 objects being used throughout the signing process
For a V1Beta1 object using the v1beta1 controller, the flow has 4 main steps:
annotations
that chains might need to use later when re-converting..Spec.Resources
,.Spec.ResourceResults
, or.Status.TaskRunStatusField.TaskSpec.Resources
we convert the V1 object back to V1Beta1 (using Tekton Pipeline conversion lib + bespoke deserialization code) and extract the necessary information as was done prior. Then the V1 Provenance is uploaded.The changes required were broadly the following:
annotations
AND converting v1beta1 for SLSA format compatibility in v1, v2, v2alpha1 & v2alpha2)updateFrom review it was preferred to keep these versionedobjects.go
TektonObject
interface to be more generic to support to v1beta1 and v1 objects (some interface types were versioned @ HEAD -Result
&Provenance
). Making the controller more natively support v1beta1 and v1 is non-trivial as in many places the TektonObject is cast back to a versioned type (eg: v1, or @ priort-to-this-pr v1beta1) for reading fields directly, etcOpen Questions:
storage/*
to use v1beta1 as well asformat/*
(v1, v2alpha1, v2alpha2)?obj
which is versioned ->StorePayload(ctx context.Context, obj objects.TektonObject, rawPayload []byte, signature string, opts config.StorageOpts) error
.Status.*
value vs their.Spec
value as the.Status
value is fully resolved from parameters, etc. In practice this means that usingConvertFrom
to go v1 -> v1beta1 to pickup some of these old fields doesn't work as intended and this cannot simply be fixed by using the.Spec
value (vs.Status
) as they are different (one is fully resolved). Currently the submitted code introduces new annotations or .Spec to .Status mapping to keep as many pre-existing tests as possible as there were prior but likely either the Pipeline conversion functions should be updated upstream to allow for this backwards compat OR the custom annotation logic in the PR here should be dropped outright. This would likely mean removing some of these old and likely deprecated tests as well