Use apis.HTTP() by skonto · Pull Request #3830 · knative/eventing

skonto · 2020-08-13T14:29:06Z

Proposed Changes

Use apis.HTTP() instead of apis.ParseURL() in the code base.
All instances are replaced except from a few where ParseURL is a better fit eg. when we want to check the uri format and report an error or do a parsing to fill in URI's fields like scheme, host, path etc.
Left untouched:

$grep -R "apis.ParseURL(" ./
./test/lib/resources/eventing.go:	apisURI, _ := apis.ParseURL(uri)
./test/lib/resources/eventing.go:	apisURI, _ := apis.ParseURL(uri)
./pkg/mtbroker/filter/filter_handler_test.go:					url, err := apis.ParseURL(s.URL)
./pkg/inmemorychannel/message_dispatcher_test.go:	url, err := apis.ParseURL(str)
./pkg/apis/sources/v1alpha1/apiserver_lifecycle.go:		if u, err := apis.ParseURL(uri); err != nil {
./pkg/apis/sources/v1alpha1/apiserver_lifecycle.go:	if u, err := apis.ParseURL(uri); err != nil {
./pkg/apis/sources/v1beta1/ping_lifecycle_test.go:	exampleUri, _ := apis.ParseURL("uri://example")
./pkg/apis/sources/v1beta1/ping_lifecycle_test.go:	exampleUri, _ := apis.ParseURL("uri://example")
./pkg/apis/sources/v1beta1/ping_lifecycle_test.go:	exampleUri, _ := apis.ParseURL("uri://example")
./pkg/apis/sources/v1alpha2/ping_lifecycle_test.go:	exampleUri, _ := apis.ParseURL("uri://example")
./pkg/apis/sources/v1alpha2/ping_lifecycle_test.go:	exampleUri, _ := apis.ParseURL("uri://example")
./pkg/apis/sources/v1alpha2/ping_lifecycle_test.go:	exampleUri, _ := apis.ParseURL("uri://example")
./pkg/apis/sources/v1alpha2/apiserver_lifecycle_test.go:	sink, _ := apis.ParseURL("uri://example")
./pkg/apis/sources/v1alpha2/apiserver_lifecycle_test.go:	sink, _ := apis.ParseURL("uri://example")
./pkg/reconciler/mtbroker/trigger/trigger_test.go:	u, _ := apis.ParseURL(sinkURI)
./pkg/reconciler/source/duck/duck_test.go:	ceSourceURL, _ := apis.ParseURL(ceSource)
./pkg/reconciler/source/duck/duck.go:		sourceURL, err := apis.ParseURL(attrib.Source)
./pkg/reconciler/source/duck/duck.go:		schemaURL, err := apis.ParseURL(schema)
./pkg/reconciler/testing/v1beta1/trigger.go:	uri, _ := apis.ParseURL(rawurl)
./pkg/reconciler/testing/v1beta1/trigger.go:	uri, _ := apis.ParseURL(rawuri)
./pkg/reconciler/testing/v1beta1/trigger.go:		u, _ := apis.ParseURL(uri)
./pkg/reconciler/testing/v1/trigger.go:	uri, _ := apis.ParseURL(rawurl)
./pkg/reconciler/testing/v1/trigger.go:	uri, _ := apis.ParseURL(rawuri)
./pkg/reconciler/testing/v1/trigger.go:		u, _ := apis.ParseURL(uri)
./pkg/reconciler/pingsource/resources/receive_adapter_test.go:	exampleUri, _ := apis.ParseURL("uri://example")

knative-prow-robot · 2020-08-13T14:29:20Z

Welcome @skonto! It looks like this is your first PR to knative/eventing 🎉

knative-prow-robot · 2020-08-13T14:29:20Z

Hi @skonto. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

pierDipi · 2020-08-13T15:19:39Z

/ok-to-test

lionelvillard · 2020-08-13T21:04:08Z

/test pull-knative-eventing-integration-tests

knative-test-reporter-robot · 2020-08-13T21:47:32Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-eventing-integration-tests	2020-08-13 16:24:29.258 +0000 UTC	1/3

Failed non-flaky tests preventing automatic retry of pull-knative-eventing-integration-tests:

test/e2e.TestBrokerChannelFlowV1Beta1BrokerV1/InMemoryChannel-messaging.knative.dev/v1beta1
test/e2e.TestBrokerChannelFlowV1Beta1BrokerV1/InMemoryChannel-messaging.knative.dev/v1
test/e2e.TestBrokerChannelFlowV1Beta1BrokerV1/Channel-messaging.knative.dev/v1beta1
test/e2e.TestBrokerChannelFlowTriggerV1BrokerV1/Channel-messaging.knative.dev/v1beta1
test/e2e.TestBrokerChannelFlowTriggerV1BrokerV1/InMemoryChannel-messaging.knative.dev/v1
test/e2e.TestBrokerChannelFlowTriggerV1BrokerV1/InMemoryChannel-messaging.knative.dev/v1beta1
test/e2e.TestBrokerChannelFlowTriggerV1Beta1BrokerV1Beta1/InMemoryChannel-messaging.knative.dev/v1beta1
test/e2e.TestBrokerChannelFlowTriggerV1Beta1BrokerV1Beta1/Channel-messaging.knative.dev/v1

and 6 more.

pierDipi

I think there are some changes in the meaning of some replacements, for example here https://github.com/knative/eventing/pull/3830/files#diff-ab0df0c2544da80554e8826de8aa0d11R404

skonto · 2020-08-24T07:58:23Z

@pierDipi correct will fix.

skonto · 2020-08-25T09:45:02Z

unit tests pass locally but the CI reports:

=== Failed
=== FAIL: pkg/reconciler/inmemorychannel/dispatcher  (0.00s)
=== CONT  
    testing.go:954: race detected during execution of test
FAIL
FAIL	knative.dev/eventing/pkg/reconciler/inmemorychannel/dispatcher	4.281s

skonto · 2020-08-25T09:45:23Z

/retest

skonto · 2020-08-25T09:57:08Z

@pierDipi fixed cases where a raw uri needs to be parsed, LMKWYT.

pierDipi · 2020-08-27T07:38:51Z

/assign

pierDipi

Thanks @skonto!

I left some comments.

pierDipi · 2020-08-27T08:00:53Z

-	sink, _ := apis.ParseURL("uri://example")
+	sink := apis.HTTP("example")
+	sink.Scheme = "uri"


We set the scheme to http and then reset it to uri.
I think it's better to leave as it was before because the scheme isn't HTTP.

Initially I thought from the description that we should use HTTP() everywhere but it is not the case. I will update it.

pierDipi · 2020-08-27T08:01:17Z

-	sink, _ := apis.ParseURL("uri://example")
+	sink := apis.HTTP("example")
+	sink.Scheme = "uri"


pierDipi · 2020-08-27T08:07:30Z

+	// Set path
+	sinkTargetURI.Path = sinkURIReference


I found complicated figuring out the full URL because it's not set in one place.
What if we use something like:

sinkTargetURI = func() apis.URL { u := apis.HTTP(sinkDNS) u.Path = sinkURIReference return u }()

or

sinkTargetURI = targetURL(sinkDNS, sinkURIReference) // ... func targetURL(host, path string) apis.URL { u := apis.HTTP(host) u.Path = path return u }

pierDipi · 2020-08-27T08:08:58Z

-	exampleUri, _ := apis.ParseURL("uri://example")
+	exampleUri := apis.HTTP("example")
+	exampleUri.Scheme = "uri"


skonto · 2020-08-28T08:40:35Z

@pierDipi I updated the PR (also there were a couple of more places where I was setting the scheme in a different statement).

pierDipi

Thank you!

/lgtm

lionelvillard · 2020-08-28T13:29:35Z

neat!
/approve

knative-prow-robot · 2020-08-28T13:29:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lionelvillard, pierDipi, skonto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [lionelvillard]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 13, 2020

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 13, 2020

knative-prow-robot requested review from steuhs and vaikas August 13, 2020 14:29

knative-prow-robot added area/test-and-release Test infrastructure, tests or release needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 13, 2020

skonto changed the title ~~Use apis.HTTP()~~ [WIP]Use apis.HTTP() Aug 13, 2020

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 13, 2020

skonto force-pushed the apis_http branch 4 times, most recently from c5d0352 to 1738cf4 Compare August 13, 2020 15:15

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 13, 2020

skonto force-pushed the apis_http branch from 1738cf4 to 39f73b9 Compare August 13, 2020 15:22

skonto changed the title ~~[WIP]Use apis.HTTP()~~ Use apis.HTTP() Aug 13, 2020

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 13, 2020

pierDipi reviewed Aug 14, 2020

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2020

Stavros Kontopoulos added 2 commits August 25, 2020 11:56

use apis.HTTP

20678a7

fixes

609bd61

skonto force-pushed the apis_http branch from 5b5f1d9 to 609bd61 Compare August 25, 2020 08:57

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2020

knative-prow-robot assigned pierDipi Aug 27, 2020

pierDipi reviewed Aug 27, 2020

View reviewed changes

updates

d022ca3

pierDipi approved these changes Aug 28, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 28, 2020

knative-prow-robot merged commit daaf561 into knative:master Aug 28, 2020

pierDipi mentioned this pull request Sep 22, 2020

Add pierDipi to approvers #4095

Merged

Conversation

skonto commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot commented Aug 13, 2020

Uh oh!

knative-prow-robot commented Aug 13, 2020

Uh oh!

pierDipi commented Aug 13, 2020

Uh oh!

lionelvillard commented Aug 13, 2020

/test pull-knative-eventing-integration-tests

Uh oh!

knative-test-reporter-robot commented Aug 13, 2020

Uh oh!

pierDipi left a comment

Choose a reason for hiding this comment

Uh oh!

skonto commented Aug 24, 2020

Uh oh!

skonto commented Aug 25, 2020

Uh oh!

skonto commented Aug 25, 2020

Uh oh!

skonto commented Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pierDipi commented Aug 27, 2020

Uh oh!

pierDipi left a comment

Choose a reason for hiding this comment

Uh oh!

pierDipi Aug 27, 2020

Choose a reason for hiding this comment

Uh oh!

skonto Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierDipi Aug 27, 2020

Choose a reason for hiding this comment

Uh oh!

pierDipi Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierDipi Aug 27, 2020

Choose a reason for hiding this comment

Uh oh!

skonto commented Aug 28, 2020

Uh oh!

pierDipi left a comment

Choose a reason for hiding this comment

Uh oh!

lionelvillard commented Aug 28, 2020

Uh oh!

knative-prow-robot commented Aug 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

skonto commented Aug 13, 2020 •

edited

Loading

skonto commented Aug 25, 2020 •

edited

Loading

skonto Aug 27, 2020 •

edited

Loading

pierDipi Aug 27, 2020 •

edited

Loading