removing parse url from the get rabbitmqUrl function

[gabo1208]

Changes

• 🎁 Now Brokers and Sources accept broader rabbitmq url usernames and passwords characters to connect to RabbitMQ instances

/kind bug

Fixes #1216

Release Note

Now Brokers and Sources accept broader rabbitmq url usernames and passwords characters to connect to RabbitMQ instances

[codecov]

Codecov Report

Merging #1239 (0ecb20d) into main (a58af36) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is 53.84%.

@@           Coverage Diff           @@
##             main    #1239   +/-   ##
=======================================
  Coverage   74.75%   74.75%           
=======================================
  Files          44       44           
  Lines        3430     3430           
=======================================
  Hits         2564     2564           
  Misses        765      765           
  Partials      101      101           

Files	Coverage Δ
pkg/rabbit/exchange.go	100.00% <ø> (ø)
pkg/reconciler/broker/broker.go	86.26% <100.00%> (ø)
pkg/rabbit/service.go	29.25% <50.00%> (ø)

[gabo1208]

@kvmware @dprotaso here I need to ignore the test coverage since it is not affecting anything reported by codecov

[dprotaso]

/override "codecov/patch"

@gabo1208 you should figure out how to configure the codecov so we don't need to override

I think the LGTM is what is missing here, you are right.

[knative-prow]

@dprotaso: Overrode contexts on behalf of dprotaso: codecov/patch

In response to this:

/override "codecov/patch"

@gabo1208 you should figure out how to configure the codecov so we don't need to override

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.

[dprotaso]

it also looks like it's not mandatory so you don't need to override it - so tide just needs an /lgtm

[gabo1208]

/cherry-pick release-1.11

[knative-prow-robot]

@gabo1208: once the present PR merges, I will cherry-pick it on top of release-1.11 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.11

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.

[dolfolife]

I wonder if doing url.QueryEscape on the username might be better than us not constructing (validating a URL)
For example https://go.dev/play/p/ol-loHQ3pRn

[ChunyiLyu]

Just to check my understanding: the issue was user had a back slash in their credentials which is parsed as a special case by url.Parse and you’ve fixed it by removing url.Parse?

If you want to keep validating the URl as before, you could consider 1) adding username and password as separate fields to ExchangeArgs and keep RabbitMQURL as it is 2) construct the URL with url.URL first before converting it to string. I'm not sure if the URL is validated anywhere else. If it isn't, keeping the validation would be a safer option. You can take a look of messaging topology operator as a reference: https://github.com/rabbitmq/messaging-topology-operator/blob/main/rabbitmqclient/cluster_reference.go

It's been a while since I've looked at eventing. If the team is happy with the change please feel free to disregard my comment and go ahead 😀

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ChunyiLyu, 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:

• OWNERS [ChunyiLyu,gabo1208]

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]

@gabo1208: new pull request created: #1259

In response to this:

/cherry-pick release-1.11

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.