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.

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 "valid" condition to all resources #55

Closed
wants to merge 1 commit into from

Conversation

bobcatfish
Copy link
Collaborator

All resources will be evaluated by their controllers after they are
created to determine if they are valid or not. This PR adds a "Valid"
condition to the status of all resoruces which can be used by the
controller to inform the user of this (otherwise I think we'd have to
implement this kind of validation via admission controllers? maybe
that's better?)

This would be used by the logic that will be implemented for validation
in #33 #32 #31 #29 #30.

All resources will be evaluated by their controllers after they are
created to determine if they are valid or not. This PR adds a "Valid"
condition to the status of all resoruces which can be used by the
controller to inform the user of this (otherwise I think we'd have to
implement this kind of validation via admission controllers? maybe
that's better?)

This would be used by the logic that will be implemented for validation
in #33 #32 #31 #29 #30.
@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 18, 2018
@bobcatfish
Copy link
Collaborator Author

Particularly interested in hearing whether folks think it would be more appropriate for validation to be implemented in each type's controller, or if it would make more sense to create admission controllers for this.

This is to check things like:

  • The github repo source really is a github repo
  • Sometimes fields will be "optional", but you should have at least one specified
  • References to other resources are actually valid references

@nader-ziada
Copy link
Member

Using Validating Admission Webhook is what I have seen in similar situations.

Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add status pipeline and pipeline params?
How about we do not install them on your cluster if they are not valid?

Sorry ,i have to go but i will take a deeper look when i am back to work.

@bobcatfish
Copy link
Collaborator Author

@pivotal-nader-ziada that backs up what @mattmoor said as well:

Use webhook. See apis.Validatable
there is a lot of good stuff in knative/pkg you should leverage for the Pipelines controller

And I think that would take care of @tejal29 's point as well:

How about we do not install them on your cluster if they are not valid?

Since the webhook would prevent the resources from being installed.

So I'll close this PR then!

Note: I was hoping that using webhooks would mean we didn't have to block implementing validation on the kubebuilder decision (#19) but it looks like kubebuilder provides libs for webhooks as well: https://book.kubebuilder.io/beyond_basics/sample_webhook.html

@bobcatfish bobcatfish closed this Sep 18, 2018
@tejal29
Copy link
Contributor

tejal29 commented Sep 18, 2018

sounds good! yes we can have webhook admission controllers.

pradeepitm12 pushed a commit to pradeepitm12/pipeline that referenced this pull request Jan 28, 2021
Modify EventListener and TriggerBinding such that TriggerBindings can be
reused. This was proposed and discussed in tektoncd#55 .

Instead of specifying a particular TriggerTemplate within a
TriggerBinding (as was previously proposed), users now specify
one-to-one relationships between TriggerBindings and TriggerTemplates
within an EventListener. Catalogs of TriggerTemplates can exist and be
reused with this change.

Although adding Params may be something we want to do for reusability in
the future, particularly for the case where filtering/conditions are
embeded within the TriggerBinding, it is out of this PR's scope. We may
separate the conditional/filtering logic in which case parameters would
make more sense there anyway.

Co-authored-by: Vincent-DeSousa-Tereso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants