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

Updated eventing to have new filters in beta #3366

Merged
merged 9 commits into from Oct 4, 2023

Conversation

Cali0707
Copy link
Member

Part of #3363

Proposed Changes

  • Update the eventing dependency to the commit where we moved the new filters to enabled by default and beta

Release Note

New trigger filters are now beta and enabled by default.

Docs

@knative-prow knative-prow bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 29, 2023
@Cali0707
Copy link
Member Author

/cc @pierDipi

@Cali0707 Cali0707 closed this Sep 29, 2023
@Cali0707 Cali0707 reopened this Sep 29, 2023
@Cali0707
Copy link
Member Author

/hold

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 29, 2023
@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

Merging #3366 (05633e9) into main (1ddc823) will increase coverage by 9.35%.
Report is 2 commits behind head on main.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #3366      +/-   ##
============================================
+ Coverage     61.52%   70.88%   +9.35%     
  Complexity      762      762              
============================================
  Files           181       90      -91     
  Lines         12314     3077    -9237     
  Branches        265      265              
============================================
- Hits           7576     2181    -5395     
+ Misses         4142      730    -3412     
+ Partials        596      166     -430     
Flag Coverage Δ
java-unittests 70.88% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 91 files with indirect coverage changes

…s so that the update would fetch latest changes

Signed-off-by: Calum Murray <cmurray@redhat.com>
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow knative-prow bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 2, 2023
@pierDipi
Copy link
Member

pierDipi commented Oct 2, 2023

@Cali0707 you need to re-run hack/update-codegen.sh

Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow knative-prow bot added area/control-plane and removed lgtm Indicates that a PR is ready to be merged. labels Oct 2, 2023
@Cali0707
Copy link
Member Author

Cali0707 commented Oct 2, 2023

/cc @pierDipi
I updated the codegen

@knative-prow knative-prow bot requested a review from pierDipi October 2, 2023 14:15
Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
Copy link
Member Author

Cali0707 commented Oct 2, 2023

@pierDipi @creydr sacura was unable to as triggers would not come ready due to the OIDC status not being set. I added a temporary implementation that just sets the status when OIDC is disabled so that the tests can run. Is this okay, or should I be doing a full implementation of OIDC here?

Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
@pierDipi
Copy link
Member

pierDipi commented Oct 3, 2023

/test reconciler-tests

Signed-off-by: Calum Murray <cmurray@redhat.com>
@@ -129,6 +129,7 @@ func namespacedTriggerReconciliation(t *testing.T, format string, env config.Env
withTriggerSubscriberResolvedSucceeded(contract.DeliveryOrder_UNORDERED),
withTriggerStatusGroupIdAnnotation(TriggerUUID),
reconcilertesting.WithTriggerDeadLetterSinkNotConfigured(),
reconcilertesting.WithTriggerOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled(),
Copy link
Member

Choose a reason for hiding this comment

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

I just realized how long this function name is

@pierDipi
Copy link
Member

pierDipi commented Oct 4, 2023

/approve

Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
@@ -129,7 +129,7 @@ func namespacedTriggerReconciliation(t *testing.T, format string, env config.Env
withTriggerSubscriberResolvedSucceeded(contract.DeliveryOrder_UNORDERED),
withTriggerStatusGroupIdAnnotation(TriggerUUID),
reconcilertesting.WithTriggerDeadLetterSinkNotConfigured(),
reconcilertesting.WithTriggerOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled(),
reconcilertesting.WithTriggerOIDCIdentityCreatedNotSupported(),
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this call, it's done in reconcilertesting.WithInitTriggerConditions

@@ -87,6 +87,7 @@ type Reconciler struct {
}

func (r *Reconciler) ReconcileKind(ctx context.Context, trigger *eventing.Trigger) reconciler.Event {
trigger.Status.MarkOIDCIdentityCreatedNotSupported()
Copy link
Member

Choose a reason for hiding this comment

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

Do we have an issue tracking this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just made #3378

Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2023
@knative-prow
Copy link

knative-prow bot commented Oct 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, pierDipi

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

Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2023
@Leo6Leo
Copy link
Contributor

Leo6Leo commented Oct 4, 2023

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2023
@Cali0707
Copy link
Member Author

Cali0707 commented Oct 4, 2023

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 4, 2023
@Cali0707
Copy link
Member Author

Cali0707 commented Oct 4, 2023

/retest-required

1 similar comment
@Cali0707
Copy link
Member Author

Cali0707 commented Oct 4, 2023

/retest-required

@knative-prow knative-prow bot merged commit 9d3559a into knative-extensions:main Oct 4, 2023
34 of 36 checks passed
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. area/control-plane area/test lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants