-
Notifications
You must be signed in to change notification settings - Fork 600
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
fixing that triggers can actually have no filters... #2700
fixing that triggers can actually have no filters... #2700
Conversation
@matzew can you add at least a unit test for this? |
for k, v := range *source.Spec.Filter.Attributes { | ||
sink.Spec.Filter.Attributes[k] = v | ||
} | ||
} | ||
if source.Spec.Filter.DeprecatedSourceAndType != nil { | ||
sink.Spec.Filter = &v1beta1.TriggerFilter{ | ||
Attributes: make(v1beta1.TriggerFilterAttributes, 0), |
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.
could we not just make this slice at least two already given that we default to adding a source
and type
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.
Could we add unit tests to make sure we don't regress? I see there's one testing without Trigger, wondering why it doesn't catch this.
} | ||
if source.Spec.Filter.Attributes != nil { | ||
sink.Spec.Filter = &v1beta1.TriggerFilter{ | ||
Attributes: make(v1beta1.TriggerFilterAttributes, 0), |
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.
instead of 0, how about?
len(source.Spec.Filter.Attributes)
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: matzew, vaikas 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 |
Fixes #2695 (and is a backport of 2696 to the
knative:release-0.13
branch)Proposed Changes
TriggerFilter{}
, and not requiring actual attributes in thereRelease Note
Docs