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

fixing bug in name of Solender to avoid CLA troubles, is fixing … #1147

Merged

Conversation

gabo1208
Copy link
Contributor

Changes

  • 🐛 Fix bug causing trigger subscriber ref namespace to be overwritten if provided

/kind bug

Fixes #1117

this PR is from @erictg to avoid getting this stale because of the CLA errors

…the triger subscriber namespace bug in case the subscriber has a different namespace from the trigger
@knative-prow knative-prow bot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 27, 2023
@knative-prow
Copy link

knative-prow bot commented Jun 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabo1208

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 knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 27, 2023
@knative-prow knative-prow bot requested review from matzew and zroubalik June 27, 2023 15:26
@knative-prow knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 27, 2023
@erictg
Copy link

erictg commented Jun 27, 2023

@gabo1208 in the pr title, can you fix the spelling of my last name? It's "Solender" :)

@gabo1208 gabo1208 changed the title fixing bug in name of Eric Solander to avoid CLA troubles, is fixing … fixing bug in name of Solender to avoid CLA troubles, is fixing … Jun 27, 2023
@gabo1208
Copy link
Contributor Author

haha sure thing! My bad, now it's fine :)

@erictg
Copy link

erictg commented Jun 27, 2023

Glad this is getting merged, I've been running a fork in prod for a few weeks now 😆

@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #1147 (8af3a2b) into main (eedae94) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1147      +/-   ##
==========================================
- Coverage   74.18%   74.14%   -0.04%     
==========================================
  Files          44       44              
  Lines        3304     3299       -5     
==========================================
- Hits         2451     2446       -5     
  Misses        764      764              
  Partials       89       89              
Impacted Files Coverage Δ
pkg/reconciler/trigger/trigger.go 63.01% <ø> (-0.47%) ⬇️

@gabo1208
Copy link
Contributor Author

Yeah sometimes CLA can be a pain, but lets get this done :) with luck maybe today we'll have the approvals

@erictg
Copy link

erictg commented Jun 27, 2023

Saw the codecov failed, those lines are tested by the unit tests I added. Not sure why codecov missed it. The 2 unit tests I added check the not defined path and different namespaces path.

Key: testKey,
Objects: []runtime.Object{
ReadyBroker(config),
makeSubscriberAddressableAsUnstructured(),
Copy link

Choose a reason for hiding this comment

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

This function https://github.com/knative-sandbox/eventing-rabbitmq/pull/1147/files#diff-c5ee2c23a8a6dd4c7aa3e6620d57f2bfd00a6b37dd072ade8bfd386dd4952d21L1406 populates a namespace. Therefor the codecoverage is not being covered in the line specified by codecov.

Copy link

Choose a reason for hiding this comment

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

@gabo1208
Copy link
Contributor Author

The weird thing is that here it was passing @kvmware #1118

@krsna-m
Copy link

krsna-m commented Jun 27, 2023

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 27, 2023
@krsna-m
Copy link

krsna-m commented Jun 27, 2023

/retest
KinD conformance tests / run (v1.25.9) was 18min stuck so cancelled and now re-running them

@knative-prow knative-prow bot merged commit 05bea06 into knative-extensions:main Jun 27, 2023
22 checks passed
@jonirap jonirap mentioned this pull request Jul 25, 2023
@gabo1208
Copy link
Contributor Author

/cherry-pick release-1.10

@gabo1208
Copy link
Contributor Author

/cherry-pick release-1.9

@knative-prow-robot
Copy link

@gabo1208: new pull request created: #1180

In response to this:

/cherry-pick release-1.10

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.

@knative-prow-robot
Copy link

@gabo1208: new pull request created: #1181

In response to this:

/cherry-pick release-1.9

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.

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. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trigger unable to get subscriber uri across namespaces
4 participants