-
Notifications
You must be signed in to change notification settings - Fork 113
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
API Review of v1alpha1 #516
Comments
👍 |
As discussed today in the Slack, we might also want to take this opportunity to audit the permissions assigned to the controller's Service Account. When we did this in Tekton, we discovered some permissions we didn't actually need, and some which we only needed in Tekton's own namespace, instead of cluster-wide. |
I´m adding an initial list of API changes from a brief recollection of issues. This is categorized by This is a living table, changes are expected to happen
|
On going from alpha to beta, I'll put a cookie crumb link to https://kubernetes.slack.com/archives/C019ZRGUEJC/p1614018974009600 in slack around some of the pain upstream tekton has run into wrt supporting alpha clients once their API went beta and more fields were added. As we discussed in the Mar 1 2021 community meeting today, we should review the k8s conventions / requirements around that scenario when deciding to move from alpha to beta. |
I noticed that Aside from exposing The reason you'd want to specify the fields yourself is twofold:
|
I also noticed that a number of fields in the API have a different name in the Go struct than in the JSON/YAML, such as: type BuildSpec struct {
Source GitSource `json:"source"`
StrategyRef *StrategyRef `json:"strategy"`
BuilderImage *Image `json:"builder,omitempty"`
...
} (ref) In this case, it can be hard to read the Go struct and understand what the corresponding YAML should look like. Is it This can become painful when you're trying to write Go code against the API, since you have to keep both names in mind all the time. Maybe your IDE helps you, but we shouldn't require an IDE to have a good time. This could be aligned in a backward-compatible* change, by renaming the Go fields/structs to align with the JSON/YAML struct tags. This could be a good-first-issue even on the pre-beta timeline. *Tekton has defined backward compatibility to mean "YAML/JSON that used to work still works", not necessarily that Go code still compiles. edit: also a couple examples of |
+1 |
Possibly #224 ? |
We were going back and forth on this in the initial days. Fixing this sounds like a good thing to do.
Agreed. |
Actually a different one :( |
@adambkaplan wondering if this one should not belong to the v0.4.0 milestone, it feels like this is a "living" issue that should continue open for more weeks? |
I agree @qu1queee . I'm moving it out while Adam is on PTO. If he feels strongly about it when he gets back, we can go from there. |
@gabemontero also points out that
We use |
Small clarification ... I believe
It is only |
I think the guidance is still reasonable, especially:
paired with
That TODO makes me worried that This is basically just another instance of the problem outlined in #516 (comment) -- we shouldn't use k8s built-in types we don't control. It's easier to make this switch sooner rather than later. |
@imjasonh from your comment, what does |
Tekton supports a https://tekton.dev/docs/pipelines/tasks/#running-scripts-within-steps I agree about moving off v1.Container not being high priority, but I do think it should be relatively non-distruptive to users, so I don't think we need to hold it for GA/v1 necessarily. |
From grooming:
|
This started in #662 and resulted in #679 Generating service accounts is an unusual feature, which requires users grant Shipwright massive permissions inside the cluster, and should be deprecated before we consider the v1alpha1 API to be stable and close this issue. |
Done with https://hackmd.io/BsPjvGFwQgmVeJRe6FJfMA |
As this project progresses, several outdated/obsolete aspects of the Build APIs have been uncovered. This will serve as a master tracking issue of API fields that should be removed in
v1alpha1
. We will consider this issue "done" when we have reviewed the current API and agreed that all removals from our initial implementation are complete. This does not necessarily mean that we consider Shipwright's API stable enough for a beta version.The description of this field will be updated with accepted API changes.
status.succeeded
andstatus.reason
.The text was updated successfully, but these errors were encountered: