-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 Tekton-owned Step, StepTemplate, and Sidecar #4778
Conversation
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
595e908
to
a0e12f2
Compare
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.
|
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.
Really quick review but few questions:
- Would it make sense to have two different struct for sidecars and steps ? (sidecars container would be step container + some things like readinessprobe, *probe …)
- I see
v1alpha1.Container
, any reason for changing code inv1alpha1
? (I guess it might be because we "share" theStep
struct)
Yes that's definitely where I see this going but I didn't want to remove any fields of the Step container yet (might need a TEP). But I can create two identical structs for sidecars and steps that we can later change, what do you think?
Yeah pretty much because we share the Step struct. I could update the v1alpha1 Step to just copy over all the fields of the v1beta1 step but then we have to keep them in sync. Open to doing that though. |
The following is the coverage report on the affected files.
|
fwiw, I'd lean towards copying over to v1alpha1 normally, but I'm not sure if it's worth it given that TEP-0105 is starting to come together for actually removing v1alpha1. Given that, the additional work involved in copying over and keeping in sync for the time being may not be worth it. |
The following is the coverage report on the affected files.
|
Happy to discuss if folks disagree, but in general I don't think deprecating a "feature" like, e.g., If we think eventually we'd like to get to a spot where the |
The following is the coverage report on the affected files.
|
That's fair-- I wasn't sure if it would be more work to continue to alias v1alpha1 types to v1beta1 types, or to copy the old v1beta1 struct over to v1alpha1 so I wouldn't have to modify the v1alpha1 tests. I already did most of the grunt work though for pipelines -- when you're talking about the work to support this are you anticipating that this will be more difficult for the CLI team?
Point taken! I'd like to keep the scope of this PR to just swapping over the type definitions, and not deprecating anything. However, I created separate structs for Step and Sidecar containers to hopefully minimize churn. There's still a lot of copypasta so LMK if you'd prefer to see it done differently. |
tests pass! 💪 /assign @vdemeester |
sounds good-- I think this PR should be good to go then! |
The following is the coverage report on the affected files.
|
I think with this latest change we should just drop the |
Oh my bad, I understand now what you were suggesting. Will do, I'll comment here when I've made that change. |
I'm thinking of keeping StepContainer so it can be reused in StepTemplate, but copying the container fields into sidecar as well, thoughts? |
Good question. Are there fields that make sense in a |
@@ -139,7 +139,7 @@ type TaskResult struct { | |||
// Step embeds the Container type, which allows it to include fields not | |||
// provided by Container. | |||
type Step struct { | |||
corev1.Container `json:",inline"` | |||
StepContainer `json:",inline"` |
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.
The idea was to move all that is to not have this inlined/embedded struct but have the field directly in. So no need for StepContainer
.
@@ -172,7 +172,7 @@ type Step struct { | |||
|
|||
// Sidecar has nearly the same data structure as Step, consisting of a Container and an optional Script, but does not have the ability to timeout. | |||
type Sidecar struct { | |||
corev1.Container `json:",inline"` | |||
SidecarContainer `json:",inline"` |
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.
The idea was to move all that is to not have this inlined/embedded struct but have the field directly in. So no need for SidecarContainer
.
I think "Name" wouldn't make sense in StepTemplate since steps can't have the same name. In addition, "timeout" is not currently present in StepTemplate, but it could make sense. so maybe these fields should be directly copied into stepTemplate as well, thoughts? |
Thanks for looking into it. It sounds like we should just copy these between the types that need them. I'm not sure we should add |
Agreed! I don't want to change the user facing API at all with this PR-- just mentioning timeout as something that could be added in the future. |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
@vdemeester @imjasonh I've addressed your feedback |
The following is the coverage report on the affected files.
|
This commit copies the Container struct from Kubernetes into Tekton into the existing structs Task.Step.Container, Task.Sidecar.Container, and Task.StepTemplate. This change breaks the Tekton Go libraries, but does not require any changes to the yaml that users provide. This should not change anything for users who do not use the Go libraries. The reason for this change is to have more control over our Task API. For example, we may want to modify the Container fields we support, or change fields to string types to allow them to be parameterized. This commit is the first step towards being able to make changes to parts of the Task API embedded in Container.
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
Thanks for this!
Happy to help! /test pull-tekton-pipeline-alpha-integration-tests |
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.
/meow
In response to this:
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. |
[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:
Approvers can indicate their approval by writing |
Changes
This commit copies the Container struct from Kubernetes into Tekton into the
existing structs Task.Step.Container, Task.Sidecar.Container, and Task.StepTemplate.
This change breaks the Tekton Go libraries, but does not require any changes to the yaml
that users provide. This should not change anything for users who do not use the Go libraries.
The reason for this change is to have more control over our Task API. For example,
we may want to modify the Container fields we support (e.g. #4737), or change fields to string types
to allow them to be parameterized (e.g. #4080). This commit is the first step towards being able to make changes
to parts of the Task API embedded in Container.
Part 1 of #4737
/kind misc
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
(if there are no user facing changes, use release note "NONE")
Release Notes