Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use Knative's {Type}StatusFields Pattern #1590

Closed
poy opened this issue Nov 19, 2019 · 6 comments · Fixed by #1638
Closed

Use Knative's {Type}StatusFields Pattern #1590

poy opened this issue Nov 19, 2019 · 6 comments · Fixed by #1638
Labels
area/api Indicates an issue or PR that deals with the API. kind/feature Categorizes issue or PR as related to a new feature.

Comments

@poy
Copy link
Contributor

poy commented Nov 19, 2019

Expected Behavior

I would like to be able to embed Tekton's Status in my own CRD (e.g., for a TaskRun).

Actual Behavior

I have to recreate Tekton's Status type because I can't embed it due to the included duck.Status.

Additional Info

Knative-Serving solves this by creating a new Type that doesn't embed the knative.dev/pkg/apis/duck/v1beta1.Status.

An example of it is their ServiceStatus

Notice that it embeds both the ConfigurationStatusFields and the RouteStatusFields.

I would be happy to contribute towards this if this is something we want to do.
@imjasonh
Copy link
Member

I'm having some difficulty understanding what we're doing that's causing a problem for you. It sounds like you want to embed TaskRunStatus in your own *Status struct, is that correct? Like this?

type FooStatus struct {
  v1alpha1.TaskRunStatus `json:",inline"`
}

And the inclusion of the knative.dev/pkg/apis/duck/v1alpha1.Status type is causing problems somehow? Can you give me an example error message you're seeing?

(In general I have no problem rearranging how this type is defined, so long as it doesn't break compatibility, I'm just having difficulty understanding what's wrong with how it's described now)

@poy
Copy link
Contributor Author

poy commented Nov 19, 2019

You are correct that I'm wanting to do this. Its less of a compilation problem and more that it seems misleading (to me at least).

With Knative's pattern, it's clear what fields will be filled in. However if I embed the TaskRunStatus as it is today, it has an unused field Status. WDYT?

@poy
Copy link
Contributor Author

poy commented Nov 27, 2019

@imjasonh I'm working on a PR for this now. Do you think it would make more sense for v1alpha2 or beta?

poy added a commit to poy/pipeline that referenced this issue Nov 27, 2019
@poy
Update v1alpha1 status types to line fields
6303dd0 
Splits up the status types into two structs:

* XXXStatus
* XXXStatusFields

This first inlines both knative's duck status type and the
XXXStatusFields. The second stores all the fields that were originially
in XXXStatus. This is advantageous as downstream users can now embed a
the XXXStatusFields if their CRDs manage a Tekton object.

This pattern is demonstrated in Knative (e.g., Serving). Moving to this
pattern does not change the JSON/YAML structure in anyway, however it
does change how status objects have to be instantiated in Go.

fixes tektoncd#1590
poy added a commit to poy/pipeline that referenced this issue Nov 27, 2019
@poy
Update v1alpha1 status types to line fields
a7d2ddc 
Splits up the status types into two structs:

* XXXStatus
* XXXStatusFields

This first inlines both knative's duck status type and the
XXXStatusFields. The second stores all the fields that were originially
in XXXStatus. This is advantageous as downstream users can now embed a
the XXXStatusFields if their CRDs manage a Tekton object.

This pattern is demonstrated in Knative (e.g., Serving). Moving to this
pattern does not change the JSON/YAML structure in anyway, however it
does change how status objects have to be instantiated in Go.

fixes tektoncd#1590
@poy poy mentioned this issue Nov 27, 2019
Merged
3 tasks
@imjasonh
Copy link
Member

How much of a breaking change is it to customers who only see YAML? Can you give me an idea of the before/after for especially the YAML diff, and then less so the Go struct diff?

@imjasonh
Copy link
Member

Nevermind just saw the PR

poy added a commit to poy/pipeline that referenced this issue Nov 30, 2019
@poy
Update v1alpha1 status types to inline fields
e3c22fc 
Splits up the status types into two structs:

* XXXStatus
* XXXStatusFields

This first inlines both knative's duck status type and the
XXXStatusFields. The second stores all the fields that were originially
in XXXStatus. This is advantageous as downstream users can now embed a
the XXXStatusFields if their CRDs manage a Tekton object.

This pattern is demonstrated in Knative (e.g., Serving). Moving to this
pattern does not change the JSON/YAML structure in anyway, however it
does change how status objects have to be instantiated in Go.

fixes tektoncd#1590
@vdemeester
Copy link
Member

/kind feature
/area api

@tekton-robot tekton-robot added kind/feature Categorizes issue or PR as related to a new feature. area/api Indicates an issue or PR that deals with the API. labels Dec 9, 2019
poy added a commit to poy/pipeline that referenced this issue Dec 11, 2019
@poy
Update v1alpha1 status types to inline fields
472d78d 
Splits up the status types into two structs:

* XXXStatus
* XXXStatusFields

This first inlines both knative's duck status type and the
XXXStatusFields. The second stores all the fields that were originially
in XXXStatus. This is advantageous as downstream users can now embed a
the XXXStatusFields if their CRDs manage a Tekton object.

This pattern is demonstrated in Knative (e.g., Serving). Moving to this
pattern does not change the JSON/YAML structure in anyway, however it
does change how status objects have to be instantiated in Go.

fixes tektoncd#1590
tekton-robot pushed a commit that referenced this issue Dec 11, 2019
@poy @tekton-robot
Update v1alpha1 status types to inline fields
a90f2ae 
Splits up the status types into two structs:

* XXXStatus
* XXXStatusFields

This first inlines both knative's duck status type and the
XXXStatusFields. The second stores all the fields that were originially
in XXXStatus. This is advantageous as downstream users can now embed a
the XXXStatusFields if their CRDs manage a Tekton object.

This pattern is demonstrated in Knative (e.g., Serving). Moving to this
pattern does not change the JSON/YAML structure in anyway, however it
does change how status objects have to be instantiated in Go.

fixes #1590
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/api Indicates an issue or PR that deals with the API. kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update v1alpha1 status types to inline fields poy/pipeline
4 participants
@vdemeester @imjasonh @poy @tekton-robot