-
Notifications
You must be signed in to change notification settings - Fork 225
GitlabSource webhook that runs alongside the controller #1120
GitlabSource webhook that runs alongside the controller #1120
Conversation
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.
barring content of the Validate/SetDefaults, this generally looks good to me.
/assign @n3wscott
For Source-related defaulting guidance.
gitlab/config/201-clusterrole.yaml
Outdated
@@ -62,6 +62,8 @@ rules: | |||
- get | |||
- list | |||
- watch | |||
# Webhook controller needs it to update "sources-webhook-certs" contents |
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.
nit: we should have webhook-specific secrets to avoid things clobbering each other. This may need fixing elsewhere, but it's something I've started trying to do.
errs = errs.Also(fe) | ||
} else if fe := gs.Sink.Validate(ctx); fe != nil { | ||
errs = errs.Also(fe.ViaField("sink")) | ||
} |
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.
cc @n3wscott for wisdom around Source validation and Defaulting.
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 is a helper for sink:
// Validate sink
errs = errs.Also(cs.Sink.Validate(ctx).ViaField("sink"))
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.
also, spec.Sink
is required, so it should not be a pointer.
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.
you are using the helper 😆 sorry. But the question likely comes from the pointer type and having more logic here than needed. We can look into pushing some of the source spec validation into the ducktype too next release to help this.
Yeah ideally this would use it's own secret. |
The following is the coverage report on the affected files.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tzununbekov 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 |
/retest |
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.
Fixes #1119