Add v1beta1 Go API types #1103
Add v1beta1 Go API types #1103
Conversation
tekton-robot
added
release-note
tekton-robot
added
the
size/XXL
tekton-robot
removed
the
do-not-merge/work-in-progress
|
The following is the coverage report on the affected files.
|
|
/assign @wlynch |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-triggers-integration-tests |
| } | ||
|
|
||
| func (spec *EventListenerSpec) updatePodTemplate() { | ||
| if spec.DeprecatedPodTemplate != nil { |
There was a problem hiding this comment.
Since these fields are deprecated, can we prune them from v1beta1?
There was a problem hiding this comment.
That particular field is going away with #1105 though generally it depends - we'd have to keep supporting fields in v1beta1 that are a part of v1alpha1 due to how kube api machinery works (v1beta1 is the stored version so all fields must be supported). We'll use the validation webhook to return errors for fields that should not be supported in v1beta1 version.
There was a problem hiding this comment.
There's also other deprecated fields for interceptors, etc. My worry is if we introduce these fields, if we're strictly following the deprecation policy this effectively resets the clock for removal since we're "introducing a new field" even though it's deprecated from the start.
If I'm understanding https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#on-compatibility correctly, we don't need to have 1:1 parity with every field in the stored version - we just need to make sure users can go from v1alpha1 to v1beta1 and back without data loss.
The same way we already mutate DeprecatedPodTemplate -> spec.Resources.KubernetesResource, I don't think we need to support the deprecated fields in v1beta1, since we can safely convert back to v1alpha1 by using KubernetesResource. This is analogous to the k8s example where v6 holds (Param, Params), where as the stored v7 version only stores Params and does not have the param field - it just populates it on conversion from Params.
There was a problem hiding this comment.
I think that's a valid concern. I think there were some issues with JSON marshalling without having the fields set but let me verify. I think given the conversions we are doing in the defaulting webhook, we should not need an extra conversion webhook
There was a problem hiding this comment.
Ok, I think this will actually work!
There was a problem hiding this comment.
I assume it works because when we upgrade from old version to new version there won't be API update so if user want they will create new resources with v1beta1 am i right ?
|
Integration test failed due to an install error this time. /test pull-tekton-triggers-integration-tests |
|
The following is the coverage report on the affected files.
|
|
Here is the path I verified with the storage type set to
Also, checked the coverage reports to verify we did not lose coverage when removing old fields. One question is around what we do with the old style |
|
/retest |
|
The following is the coverage report on the affected files.
|
|
Logs say: ==== UNIT TESTS PASSED ==== 🙃 |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
tekton-robot
added
the
needs-rebase
|
/lgtm |
tekton-robot
removed
lgtm
|
The following is the coverage report on the affected files.
|
| v1beta1.SchemeGroupVersion.WithKind("ClusterTriggerBinding"): &v1beta1.ClusterTriggerBinding{}, | ||
| v1beta1.SchemeGroupVersion.WithKind("EventListener"): &v1beta1.EventListener{}, | ||
| v1beta1.SchemeGroupVersion.WithKind("TriggerBinding"): &v1beta1.TriggerBinding{}, | ||
| v1beta1.SchemeGroupVersion.WithKind("TriggerTemplate"): &v1beta1.TriggerTemplate{}, | ||
| v1beta1.SchemeGroupVersion.WithKind("Trigger"): &v1beta1.Trigger{}, |
There was a problem hiding this comment.
Based on the above changes i assume we are not adding ClusterInterceptor to beta right ?
There was a problem hiding this comment.
Yes, they will be in the v1alpha1 type but can be used from v1beta1 triggers
This commit adds v1beta1 go types and the corresponding generated code. These types cannot be used yet until we enable them in the CRDs. Part of tektoncd#1067 Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
These fields will still be supported in the v1alpha1 types.
At the moment, these are internal types and will not be usable by the end user.
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: savitaashture 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 |
tekton-robot
added
the
approved
Changes
This commit adds v1beta1 go types and the corresponding generated code. These types cannot be used yet until we enable them in the CRDs.
Part of #1067
Signed-off-by: Dibyo Mukherjee dibyo@google.com
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Release Notes