-
Notifications
You must be signed in to change notification settings - Fork 419
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
Create new types and add initial generated code #17
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: iancoffey 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 |
5af5190
to
1861927
Compare
- Adds hack/ dir with codegen scripts - adds base api types and generated client - add boilerplate.go.txt - add third_party VENDOR-LICENSE - initial dep ensure
1861927
to
7e31421
Compare
ResourceTemplates []TriggerResourceTemplate `json:"resourcetemplates,omitempty"` | ||
} | ||
|
||
//TODO: This type will need to handle creating various resource types and inlining their Specs |
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.
@ncskier @vtereso @EliZucker @bobcatfish Regarding implementing a TriggerTemplateSpec
- how do you think would should define the ResourceTemplates
part of the spec? IIRC this type will vary based on the Type being define - eg, TriggerResourceTemplate .Spec
might define a PipelineRunSpec or a PipelineResourceSpec. Am I understanding right?
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.
Since we want to support any resource,TriggerResourceTemplate
could look something like
type TriggerResourceTemplate struct {
metav1.TypeMeta `json:",inline"`
// +optional
metav1.ObjectMeta `json:"metadata,omitempty"`
Spec map[string]interface{} `json:"spec"`
Status TriggerResourceStatus `json:"status"`
}
type TriggerResourceStatus struct{}
However, I believe we might run into some issues for API resources like secrets since they don't have a spec
type Secret struct {
metav1.TypeMeta `json:",inline"`
// Standard object's metadata.
// More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#metadata
// +optional
metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`
// Data contains the secret data. Each key must consist of alphanumeric
// characters, '-', '_' or '.'. The serialized form of the secret data is a
// base64 encoded string, representing the arbitrary (possibly non-string)
// data value here. Described in https://tools.ietf.org/html/rfc4648#section-4
// +optional
Data map[string][]byte `json:"data,omitempty" protobuf:"bytes,2,rep,name=data"`
// stringData allows specifying non-binary secret data in string form.
// It is provided as a write-only convenience method.
// All keys and values are merged into the data field on write, overwriting any existing values.
// It is never output when reading from the API.
// +k8s:conversion-gen=false
// +optional
StringData map[string]string `json:"stringData,omitempty" protobuf:"bytes,4,rep,name=stringData"`
// Used to facilitate programmatic handling of secret data.
// +optional
Type SecretType `json:"type,omitempty" protobuf:"bytes,3,opt,name=type,casttype=SecretType"`
}
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.
We could do an initial parse and store pointers (add a field in the above struct) to the data that needs to be modified so we don't need to drill into the structure I mentioned in this issue.
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.
At the beginning, are we going to accept any k8s resource as a template, or only Tekton resources?
Also, would it be possible to use Go's version of a superclass for all k8s resources as the TriggerResourceTemplate
, then use the TypeMeta
to cast the resource to its proper type? For example, could we use something like the k8s Object
interface as the TriggerResourceTemplate
?
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.
As @ncskier mentioned, we should be able to route the creation through the kube API according to what is specified in the TypeMeta.
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.
When I tried Spec map[string]interface{} `json:",inline"
, it seems to unmarshal incorrectly, but maybe I made a mistake. EDIT: So the default Unmarshal does not like two fields with inline tags. Likely we will need to implement the UnmarshalJSON method. @ncskier also made a good point that if we will raw parse the object bytes at event time (TriggerBinding), it would likely be better for the "spec" field to be json.RawMessage
/[]byte
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.
At the beginning, are we going to accept any k8s resource as a template, or only Tekton resources?
I think to keep it simple we could pick a subset of resources we want to support and support those explicitly (PipelineRun, PipelineResource) - we can expand this as we go!
Or to keep it as simple as possible, in this first PR we could make it so that we only use PipelineRunSpec and add PipelineResource (and the required marshalling/unmarshalling logic) in a later PR?
or to keep it even simpler, we can commit this as is with no actual templates in here at all! :D
how do you think would should define the ResourceTemplates part of the spec?
And as for how to refer to the structure of these, I think we can import the autogenerated code from Pipelines (maybe pin to a specific release?) and use the Specs (e.g. https://github.com/tektoncd/pipeline/blob/master/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go#L44)
p.s. for marshalling/unmarshalling arbitrary types, @EliZucker is working on a type in the Pipelines repo for array params that does something similar, might be a useful reference (I think he used tektoncd/pipeline#207 (comment) as a reference?)
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.
or to keep it even simpler, we can commit this as is with no actual templates in here at all! :D
Great point - I think getting this landed and adding this Spec handling in a followup PR sounds like a good option, if that works for everyone.
For example, could we use something like the k8s Object interface as the TriggerResourceTemplate?
This is how I was thinking about this too, using runtime.Obect to accept any resource type. Or, we could just inline slices of PipelineRuns, PipelineResources, etc for every supported type. Seems like theres a few ways forward, we could do a deeper dive in a followup issue if that sounds ok to everyone.
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.
Slight change to the above types that we might want to consider:
// Used to set TriggerResourceTemplate meta structs
// Prevents recursive stack overflow
type MetaResource struct {
metav1.TypeMeta `json:",inline"`
// +optional
metav1.ObjectMeta `json:"metadata,omitempty"`
}
type TriggerResourceTemplate struct {
metav1.TypeMeta `json:",inline"`
// +optional
metav1.ObjectMeta `json:"metadata,omitempty"`
Resource json.RawMessage `json:"template"`
Status TriggerResourceStatus `json:"status,omitempty"`
}
type TriggerResourceStatus struct{}
func (t *TriggerResourceTemplate) UnmarshalJSON(b []byte) error {
// Capture raw message to be parsed by TriggerBinding later
err := json.Unmarshal(b, &t.Resource)
if err != nil {
return err
}
// Set TypeMeta/ObjectMeta
r := new(MetaResource)
err = json.Unmarshal(b, r)
if err != nil {
return err
}
t.TypeMeta = r.TypeMeta
t.ObjectMeta = r.ObjectMeta
return nil
}
There might a slightly more clever method to accomplish this, but we can discuss this in today's call about whether we look to use a json parser or internal structs.
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.
A couple of questions here to get started. Looks good so far; I'll read through the rest of it by the end of the day 🙂
// +k8s:deepcopy-gen=true | ||
type TriggerBindingRef struct { | ||
Name string `json:"name,omitempty"` | ||
Namespace string `json:"namespace,omitempty"` |
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.
This raises a question that we haven't discussed yet: are there any qualms with allowing the EventListener
to use TriggerBindings
from other namespaces? Currently, I don't think that PipelineRef
or TaskRef
allows the use of cross-namespace Pipelines
or Tasks
respectively.
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.
hm not sure! sounds like a discussion we could start in the working group that would affect both pipelines
and triggers
?
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.
I am also not sure. My use-case will keep everything namespaced together so it is not something I feel strongly about.
|
||
// +k8s:deepcopy-gen=true | ||
type TriggerBindingSpec struct { | ||
Templates []TriggerTemplateRef `json:"templates,omitempty"` |
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.
IIRC, we mentioned that we would support references as well as embedded TriggerTemplate(s)?
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.
yep fo sho! adding embedding later on might be easier tho (and starting with just references)
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.
Yes exactly ^ I thought we could add references now and then add the embedded handling separately, to make this change smaller and (maybe?) easier to reason about what is doing what. Happy to add support for embedding templates here if thats preferred.
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.
Event string json:"event,omitempty"
Is the event
actually means event.type
?
so we don't plan to filter event here, i means use event.source
or other customized attribute?
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 writing all of this up @iancoffey! I just have a few more comments for the rest of the code
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.
Looking good! I think I repeated a few times what @ncskier said about using the Param and ParamSpec types from Pipelines
Personally I'm happy to either:
- Merge this PR as is and iterate on it
- Fix up params a bit, merge and iterate
- Vendor in Pipelines, use the Param and ParamSpec types from there, merge, iterate
I vote for separating out the discussion / logic for marshalling + unmarshalling multiple Spec types into a separate PR
/meow space
--go-header-file ${REPO_ROOT_DIR}/hack/boilerplate/boilerplate.go.txt | ||
|
||
# Make sure our dependencies are up-to-date | ||
${REPO_ROOT_DIR}/hack/update-deps.sh |
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.
between this PR and the ones that @ncskier and @vincent-pli have opened, I think we should create a separate PR that just adds the update-deps.sh and update-codegen.sh scripts if we can (or maybe there will be no conflicts?)
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.
Understood - I can split the codegen and deps stuff into another branch/PR if that is useful.
ResourceTemplates []TriggerResourceTemplate `json:"resourcetemplates,omitempty"` | ||
} | ||
|
||
//TODO: This type will need to handle creating various resource types and inlining their Specs |
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.
At the beginning, are we going to accept any k8s resource as a template, or only Tekton resources?
I think to keep it simple we could pick a subset of resources we want to support and support those explicitly (PipelineRun, PipelineResource) - we can expand this as we go!
Or to keep it as simple as possible, in this first PR we could make it so that we only use PipelineRunSpec and add PipelineResource (and the required marshalling/unmarshalling logic) in a later PR?
or to keep it even simpler, we can commit this as is with no actual templates in here at all! :D
how do you think would should define the ResourceTemplates part of the spec?
And as for how to refer to the structure of these, I think we can import the autogenerated code from Pipelines (maybe pin to a specific release?) and use the Specs (e.g. https://github.com/tektoncd/pipeline/blob/master/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go#L44)
p.s. for marshalling/unmarshalling arbitrary types, @EliZucker is working on a type in the Pipelines repo for array params that does something similar, might be a useful reference (I think he used tektoncd/pipeline#207 (comment) as a reference?)
|
||
// +k8s:deepcopy-gen=true | ||
type TriggerBindingSpec struct { | ||
Templates []TriggerTemplateRef `json:"templates,omitempty"` |
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.
yep fo sho! adding embedding later on might be easier tho (and starting with just references)
// +k8s:deepcopy-gen=true | ||
type TriggerTemplateRef struct { | ||
TemplateRef string `json:"templateref,omitempty"` | ||
Event string `json:"event,omitempty"` |
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.
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.
no filter? I always thought it is.
I saw the EventListener
could binding multiple TriggerBinding
and TriggerBinding
binding multiple TriggerTemplate
,
so I thought there will pick up some one to handle the trigger based on event
attribute.
no filter here means we expect the event received should be filter before arrive here? something like knative eventing
?
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.
yeah maybe this would be the right place to put a filter! we do want to incorporate "conditions" (from tektoncd/pipeline#27) eventually
so @iancoffey i do think we should make this a field with multiple values, at least for now - so we can add more later :D
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.
Oh great catch, yes will do.
// +k8s:deepcopy-gen=true | ||
type TriggerBindingRef struct { | ||
Name string `json:"name,omitempty"` | ||
Namespace string `json:"namespace,omitempty"` |
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.
hm not sure! sounds like a discussion we could start in the working group that would affect both pipelines
and triggers
?
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. |
@vincent-pli there is no filtering in the spec yet - but I want to update the design doc (any day now 😅 ) to allow folks to (ideally) use conditions somehow to perform filtering |
Left to do
Followup Issues
|
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 the updates @iancoffey! Here are a couple of nits I ran across.
This is updated and could use a 👀 :D |
/lgtm |
Changes
Add the types and skeleton stuff described in #10.
An attempt at adding the base types described in the doc.
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.