Port Github source to adapter/v2 #1230

MIBc · 2020-05-14T09:57:48Z

Proposed Changes

Using v2 adapter in github source.

knative-prow-robot · 2020-05-14T09:58:02Z

Hi @MIBc. 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.

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.

MIBc · 2020-05-14T09:59:51Z

/assign @slinkydeveloper

slinkydeveloper · 2020-05-14T10:05:19Z

github/pkg/adapter/adapter.go

@@ -85,10 +91,16 @@ func (a *Adapter) handleEvent(payload interface{}, hdr http.Header) error {
 	event.SetSource(a.source)
 	event.SetSubject(subject)
 	event.SetDataContentType(cloudevents.ApplicationJSON)


You don't need this line anymore

slinkydeveloper

You're not using adapter/v2, you ported to sdk-go v2. Can you check how the adapter works in gitlab and try to do the same?

matzew · 2020-05-15T11:51:15Z

/ok-to-test

antoineco · 2020-05-15T16:05:14Z

github/pkg/adapter/adapter.go

+	result := a.client.Send(context.Background(), event)
+	if !cloudevents.IsACK(result) {
+		log.Printf("Cloud Event delivery error: %v", result)
+		return fmt.Errorf("failed to send cloudevent: %v", result)


%v -> %w
so that the caller can unwrap the result if needed.

antoineco · 2020-05-15T16:09:33Z

github/pkg/adapter/adapter.go

+	if err != nil {
+		log.Fatalf("failed to create http protocol: %s", err.Error())
+	}
+	a.client, err = cloudevents.NewClient(p, cloudevents.WithUUIDs(), cloudevents.WithTimeNow())
+	if err != nil {
+		log.Fatalf("failed to create client: %s", err.Error())
+	}


We have a mix of fatal and returned errors in this function.
Since the function signature includes an error return value, we should always return the error, and let the caller fail however it wants to fail.

antoineco · 2020-05-15T16:10:24Z

github/pkg/adapter/adapter.go

@@ -47,13 +46,20 @@ type Adapter struct {
 func New(sinkURI, ownerRepo string) (*Adapter, error) {
 	a := new(Adapter)
 	var err error


You can remove this since you assign to a new err below.

slinkydeveloper

Now it looks good! @antoineco can you do another review pass please?

slinkydeveloper · 2020-05-18T16:01:00Z

github/pkg/adapter/adapter_test.go

 	}
 }

+// mockTransport is a simple fake HTTP transport
+type mockTransport func(req *http.Request) (*http.Response, error)


Looks like mockTransport is dead code

slinkydeveloper · 2020-05-18T16:03:39Z

github/pkg/adapter/adapter_test.go

-	if err != nil {
-		if diff := cmp.Diff(tc.wantErrMsg, err.Error()); diff != "" {
-			return fmt.Errorf("incorrect error (-want, +got): %v", diff)
+		if err := tc.validateAcceptedPayload(t, ceClient); err != nil {


Can you add asserts here also for ce attributes like subject, id, type, source, type?

slinkydeveloper · 2020-05-18T19:22:02Z

/hold

@MIBc can you try the source to check if it's working? I think the receive adapter implementation could break if you don't pass some/all variables like K_SINK, NAME, NAMESPACE, METRICS_DOMAIN.

Check as always the GitlabSource for this 😄

antoineco · 2020-05-18T20:36:25Z

github/pkg/adapter/adapter_test.go

-			}
-		}
+	if string(got) != string(data) {
+		return fmt.Errorf("Expected %q event to be sent, got %q", data, string(got))


data will be printed as a byte array here unless you cast it to string as well.

antoineco · 2020-05-18T20:36:43Z

github/pkg/adapter/adapter_test.go

 		if err != nil {
-			t.Fatal(err)
+			t.Error(err)


Fatal was correct. If this step fails, the rest of the test has no chance to succeed.

antoineco · 2020-05-18T20:39:39Z

github/pkg/adapter/adapter_test.go

+		req.Header.Set("X-GitHub-Event", tc.eventType)
+		req.Header.Set("X-GitHub-Delivery", eventID)


"X-GitHub-Event" -> GHHeaderEvent
"X-GitHub-Delivery" -> GHHeaderDelivery

antoineco · 2020-05-18T20:42:17Z

github/pkg/adapter/adapter_test.go

-	if err != nil {
-		if diff := cmp.Diff(tc.wantErrMsg, err.Error()); diff != "" {
-			return fmt.Errorf("incorrect error (-want, +got): %v", diff)
+		if err := tc.validateAcceptedPayload(t, ceClient); err != nil {


Instead of returning an error here just to call t.Error on it, you could simply fail inside the validateAcceptedPayload function using t.Error or t.Fatal. This is a very common pattern in tests.

antoineco · 2020-05-18T20:43:42Z

github/pkg/adapter/adapter_test.go

-	h := &fakeHandler{
-		//handler: tc.sink,
-		handler: sinkAccepted, // No tests expect the sink to do anything interesting
+func (tc *testCase) validateAcceptedPayload(t *testing.T, ce *adaptertest.TestCloudEventsClient) error {


This function is a test helper, so it should start with t.Helper() so that the lines where error occur are correctly reported in logs.

antoineco · 2020-05-18T20:44:36Z

github/pkg/adapter/adapter_test.go

 	if err != nil {
-		http.Error(w, "can not read body", http.StatusBadRequest)
-		return
+		return fmt.Errorf("Could not marshal sent payload: %v", err)


Like I said in a previous comment, it would be better to fail here by calling t.Fatal instead of returning an error.

antoineco · 2020-05-18T20:45:29Z

github/pkg/adapter/adapter_test.go

@@ -407,231 +409,108 @@ var testCases = []testCase{
 	},
 }

-// mockTransport is a simple fake HTTP transport
-type mockTransport func(req *http.Request) (*http.Response, error)
+func newTestAdapter(t *testing.T, ce cloudevents.Client) *gitHubAdapter {


The t variable is unused.

The t is used to set fake context.

Right, I didn't even see that :)

antoineco · 2020-05-18T20:46:35Z

github/pkg/adapter/adapter_test.go

 	"testing"
+	"time"

 	"knative.dev/eventing-contrib/github/pkg/apis/sources/v1alpha1"


Let's group this import together with the other third-party packages. Or even better: group all knative.dev imports together.

antoineco · 2020-05-18T20:48:24Z

github/pkg/adapter/adapter.go


-	cloudevents "github.com/cloudevents/sdk-go"
+	sourcesv1alpha1 "knative.dev/eventing-contrib/github/pkg/apis/sources/v1alpha1"


Let's group this import together with the other third-party packages. Or even better: group all knative.dev imports together.

Thank for your review. I wil think about them. :)

MIBc · 2020-05-19T14:05:14Z

/hold

@MIBc can you try the source to check if it's working? I think the receive adapter implementation could break if you don't pass some/all variables like K_SINK, NAME, NAMESPACE, METRICS_DOMAIN.

Check as always the GitlabSource for this

All right. I will test it asap.

MIBc · 2020-05-23T05:23:09Z

I have fixed the code. It can run normally. @slinkydeveloper

antoineco · 2020-05-23T06:24:25Z

/lgtm

antoineco · 2020-05-23T06:29:38Z

github/pkg/adapter/adapter_test.go

-	gotSubject := event.Context.GetSubject()
-	if tc.wantCloudEventSubject != "" && tc.wantCloudEventSubject != gotSubject {
-		return nil, fmt.Errorf("want subject %s, got %s", tc.wantCloudEventSubject, gotSubject)
+		t.Helper()


This should be inside the validateAcceptedPayload, on the very first line of the function.

slinkydeveloper

Two nits and we're ready to go

slinkydeveloper · 2020-05-25T07:36:33Z

github/pkg/adapter/adapter.go


 	cloudEventType := sourcesv1alpha1.GitHubEventType(gitHubEventType)
-	subject := subjectFromGitHubEvent(gh.Event(gitHubEventType), payload)
+	subject := subjectFromGitHubEvent(gh.Event(gitHubEventType), payload, a.logger)

 	event := cloudevents.NewEvent(cloudevents.VersionV1)


Can you remove the VersionV1 in order to use always the latest version? just invoke NewEvent without parameters

slinkydeveloper · 2020-05-25T07:42:40Z

github/pkg/adapter/adapter_test.go

-
-func (h *fakeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	body, err := ioutil.ReadAll(r.Body)
+	data, err := json.Marshal(tc.payload)


If it's a json always avoid byte array/string comparisons since the serialization order of the fields could be not deterministic, so the assertion below string(got) != string(data) could fail even if the JSONs are semantically equivalent.

Parse both tc.payload and sent event data into interface{} and then use reflect.DeepEqual or cmp.Diff:

if diff := cmp.Diff(want, got); diff != "" { t.Errorf("unexpected data (-want, +got) = %v", diff) }

You didn't parsed the json:

var want interface{} json.Unmarshal(tc.payload, &want)

etc...

slinkydeveloper · 2020-05-25T07:43:38Z

github/pkg/reconciler/source/resources/service.go

@@ -47,13 +47,24 @@ func MakeService(args *ServiceArgs) *v1.Service {
 			SecretKeyRef: args.Source.Spec.SecretToken.SecretKeyRef,
 		},
 	}, {
-		Name:  "SINK",
+		Name:  "K_SINK",


antoineco · 2020-05-25T09:22:49Z

/lgtm

Fixes: knative#1199

slinkydeveloper

/lgtm
/approve

knative-metrics-robot · 2020-05-25T09:56:09Z

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-contrib-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
github/pkg/adapter/adapter.go	96.5%	88.2%	-8.4

MIBc · 2020-05-25T11:17:36Z

/retest

slinkydeveloper · 2020-05-25T11:36:19Z

/unhold
/lgtm
/approve

knative-prow-robot · 2020-05-25T11:36:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MIBc, slinkydeveloper

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:

~~github/OWNERS~~ [slinkydeveloper]

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 May 14, 2020

knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 14, 2020

knative-prow-robot requested review from lberk and rootfs May 14, 2020 09:57

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 14, 2020

slinkydeveloper reviewed May 14, 2020

View reviewed changes

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 May 15, 2020

antoineco reviewed May 15, 2020

View reviewed changes

MIBc force-pushed the github branch from c8faad8 to 46954bc Compare May 18, 2020 15:48

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 18, 2020

slinkydeveloper reviewed May 18, 2020

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 18, 2020

antoineco reviewed May 18, 2020

View reviewed changes

MIBc force-pushed the github branch from 46954bc to 062db4c Compare May 23, 2020 05:17

knative-prow-robot assigned antoineco May 23, 2020

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

antoineco reviewed May 23, 2020

View reviewed changes

MIBc force-pushed the github branch from 062db4c to f66d785 Compare May 25, 2020 07:33

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label May 25, 2020

slinkydeveloper suggested changes May 25, 2020

View reviewed changes

MIBc force-pushed the github branch from f66d785 to e5f5bc2 Compare May 25, 2020 09:06

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

slinkydeveloper mentioned this pull request May 25, 2020

Enable Tracing in Github source #1198

Closed

Port Github source to adapter/v2

eb54651

Fixes: knative#1199

MIBc force-pushed the github branch from e5f5bc2 to eb54651 Compare May 25, 2020 09:51

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label May 25, 2020

slinkydeveloper approved these changes May 25, 2020

View reviewed changes

knative-prow-robot assigned slinkydeveloper May 25, 2020

knative-prow-robot 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 May 25, 2020

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 25, 2020

knative-prow-robot merged commit bd7b4de into knative:master May 25, 2020

MIBc mentioned this pull request Aug 28, 2020

REQUEST: New membership for MIBc knative/community#243

Closed

		req.Header.Set("X-GitHub-Event", tc.eventType)
		req.Header.Set("X-GitHub-Delivery", eventID)


		cloudevents "github.com/cloudevents/sdk-go"
		sourcesv1alpha1 "knative.dev/eventing-contrib/github/pkg/apis/sources/v1alpha1"

Port Github source to adapter/v2 #1230

Port Github source to adapter/v2 #1230

Conversation

MIBc commented May 14, 2020

Proposed Changes

knative-prow-robot commented May 14, 2020

MIBc commented May 14, 2020 • edited Loading

Choose a reason for hiding this comment

slinkydeveloper left a comment

Choose a reason for hiding this comment

matzew commented May 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slinkydeveloper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slinkydeveloper commented May 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MIBc commented May 19, 2020

MIBc commented May 23, 2020

antoineco commented May 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slinkydeveloper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoineco commented May 25, 2020

slinkydeveloper left a comment

Choose a reason for hiding this comment

knative-metrics-robot commented May 25, 2020

MIBc commented May 25, 2020

slinkydeveloper commented May 25, 2020

knative-prow-robot commented May 25, 2020

MIBc commented May 14, 2020 •

edited

Loading