gateway: add api to get commit status deliveries #460

alessandro-sorint · 2023-12-11T14:29:34Z

This patch adds an api to get commit status deliveries by project ref.

sgotti · 2023-12-12T07:58:05Z

internal/services/notification/notification_test.go

@@ -504,15 +504,15 @@ func TestCommitStatusDelivery(t *testing.T) {
 	})
 }

-func createCommitStatus(t *testing.T, ctx context.Context, ns *NotificationService, n int) *types.CommitStatus {
+func createCommitStatus(t *testing.T, ctx context.Context, ns *NotificationService, n int, projectID string) *types.CommitStatus {


rename n to something more meaningful.

sgotti · 2023-12-12T07:58:45Z

internal/services/notification/notification_test.go

+			t.Fatalf("unexpected HasMore false")
+		}
+		if len(res.CommitStatusDeliveries) != 10 {
+			t.Fatalf("unexpected 10 commit status deliveries, got %d", len(res.CommitStatusDeliveries))


unexpected or expected? Probably wrong also in the code you copied.

sgotti · 2023-12-12T07:59:48Z

internal/services/notification/notification_test.go

+			t.Fatalf("unexpected HasMore true")
+		}
+		if len(res.CommitStatusDeliveries) != 10 {
+			t.Fatalf("unexpected 10 commit status deliveries, got %d", len(res.CommitStatusDeliveries))


unexpected or expected? Probably wrong also in the code you copied.

sgotti · 2023-12-12T07:59:52Z

internal/services/notification/notification_test.go

+			t.Fatalf("unexpected HasMore false")
+		}
+		if len(res.CommitStatusDeliveries) != 5 {
+			t.Fatalf("unexpected 5 commit status deliveries, got %d", len(res.CommitStatusDeliveries))


unexpected or expected? Probably wrong also in the code you copied.

sgotti · 2023-12-12T08:01:00Z

internal/services/notification/notification_test.go

+
+	t.Logf("starting ns")
+
+	time.Sleep(1 * time.Second)


Avoid time.Sleep. Probably wrong also in the code you copied from.

sgotti · 2023-12-12T09:57:06Z

tests/setup_test.go

+
+	giteaRepo, project := createProject(ctx, t, giteaClient, gwUser01Client, withVisibility(gwapitypes.VisibilityPrivate))
+
+	push(t, config, giteaRepo.CloneURL, giteaToken, "commit", false)


since looks like you get only two commit statuses, why don't you just create more runs? In this way you can check the continuation cursor by iterating more than one time etc... like done for other tests.

sgotti · 2023-12-13T08:19:50Z

internal/services/notification/notification_test.go

@@ -496,16 +496,16 @@ func TestCommitStatusDelivery(t *testing.T) {
 	})
 }

-func createCommitStatus(t *testing.T, ctx context.Context, ns *NotificationService, n int) *types.CommitStatus {
+func createCommitStatus(t *testing.T, ctx context.Context, ns *NotificationService, index int, projectID string) *types.CommitStatus {


why "index" ? it's used for RunCounter and for other unrelated things. Which is the primary use of it? "runCounter"? The other entries (commitSHA, Context etc...) are based on runCounter? If they are meaningless for the tests then document that their value is just a dumb value.

sgotti · 2023-12-13T08:23:08Z

internal/services/notification/notification_test.go

+
+	ns := setupNotificationService(ctx, t, log, dir)
+
+	t.Logf("starting ns")


remove this log line

sgotti · 2023-12-13T08:28:17Z

tests/setup_test.go

+	runNumbers := 5
+
+	var startRunNumber uint64
+	for i := 0; i < runNumbers; i++ {
+		_ = testutil.Wait(30*time.Second, func() (bool, error) {
+			runs, _, err := gwUser01Client.GetProjectRuns(ctx, project.ID, nil, nil, startRunNumber, 0, true)
+			if err != nil {
+				return false, nil
+			}
+
+			if len(runs) == 0 {
+				return false, nil
+			}
+			if runs[0].Phase != rstypes.RunPhaseFinished {
+				return false, nil
+			}
+
+			return true, nil
+		})
+
+		runs, _, err := gwUser01Client.GetProjectRuns(ctx, project.ID, nil, nil, startRunNumber, 0, true)
+		if err != nil {
+			t.Fatalf("unexpected err: %v", err)
+		}
+		if len(runs) == 0 {
+			t.Fatalf("expected 1 run got: %d", len(runs))
+		}
+		if runs[0].Phase != rstypes.RunPhaseFinished {
+			t.Fatalf("expected run phase %q, got %q", rstypes.RunPhaseFinished, runs[0].Phase)
+		}
+		if runs[0].Result != rstypes.RunResultSuccess {
+			t.Fatalf("expected run result %q, got %q", rstypes.RunResultSuccess, runs[0].Result)
+		}
+
+		startRunNumber = runs[0].Number - 1
+
+		if i == runNumbers-1 {
+			break
+		}
+
+		if _, err = gwUser01Client.ProjectCreateRun(ctx, project.ID, &gwapitypes.ProjectCreateRunRequest{Branch: "master"}); err != nil {
+			t.Fatalf("unexpected err: %v", err)
+		}
+	}


Why don't you just call 'push' n times and check that all the runs are finished/successfull instead of 1 time + all of this code?

I did the change but it fails with setuperror, I have seen it happen because something is wrong with gitea when the runservice get the conf file.. it can be because of too many calls to gitea during the run creation(?). This is why I did it by a for cicle before. I could add a time sleep after evry ProjectCreateRun if it works?

sgotti · 2023-12-13T15:18:17Z

tests/setup_test.go

+
+	giteaRepo, project := createProject(ctx, t, giteaClient, gwUser01Client, withVisibility(gwapitypes.VisibilityPrivate))
+
+	runNumbers := 5


sgotti · 2023-12-13T15:19:00Z

tests/setup_test.go

+		push(t, fmt.Sprintf(config, i), giteaRepo.CloneURL, giteaToken, "commit", false)
+	}
+
+	_ = testutil.Wait(150*time.Second, func() (bool, error) {


150 seconds is quite a lot.

internal/services/notification/db/db.go

sgotti · 2023-12-14T09:29:13Z

internal/services/notification/db/db.go

@@ -214,11 +226,8 @@ func (d *DB) GetLastRunEventSequence(tx *sql.Tx) (*types.LastRunEventSequence, e
 	return out, errors.WithStack(err)
 }

-func (d *DB) GetCommitStatusDeliveriesAfterSequence(tx *sql.Tx, afterSequence uint64, deliveryStatus types.DeliveryStatus, limit int) ([]*types.CommitStatusDelivery, error) {
+func (d *DB) GetProjectCommitStatusDeliveriesAfterSequence(tx *sql.Tx, afterSequence uint64, limit int) ([]*types.CommitStatusDelivery, error) {


GetProject... is wrong since there's no filter by project. Probably also the function you copied has the same issue.

the other function with the same issue is GetProjectRunWebhookDeliveriesAfterSequence, should I change in this PR with GetRunWebhookDeliveriesAfterSequence or I must open a new PR?

Please open a new PR with a clear/detailed commit.

opened PR #467 to fix

add an api to get commit status deliveries by project id.

add an api to get commit status deliveries by project ref.

sgotti requested changes Dec 12, 2023

View reviewed changes

alessandro-sorint force-pushed the api-commitstatusdeliveries branch from 53be9d2 to 44c8942 Compare December 12, 2023 08:19

alessandro-sorint requested a review from sgotti December 12, 2023 08:24

sgotti requested changes Dec 12, 2023

View reviewed changes

alessandro-sorint force-pushed the api-commitstatusdeliveries branch 2 times, most recently from 6a0df88 to c88acfe Compare December 13, 2023 08:16

sgotti requested changes Dec 13, 2023

View reviewed changes

alessandro-sorint force-pushed the api-commitstatusdeliveries branch from c88acfe to 498621c Compare December 13, 2023 10:09

alessandro-sorint requested a review from sgotti December 13, 2023 10:42

alessandro-sorint force-pushed the api-commitstatusdeliveries branch 2 times, most recently from 004d0da to b84a824 Compare December 13, 2023 14:22

sgotti requested changes Dec 13, 2023

View reviewed changes

alessandro-sorint force-pushed the api-commitstatusdeliveries branch from b84a824 to 5001af4 Compare December 13, 2023 15:32

alessandro-sorint requested a review from sgotti December 13, 2023 16:29

sgotti requested changes Dec 14, 2023

View reviewed changes

internal/services/notification/db/db.go Show resolved Hide resolved

alessandro-sorint force-pushed the api-commitstatusdeliveries branch from 5001af4 to e515f66 Compare December 14, 2023 08:27

alessandro-sorint requested a review from sgotti December 14, 2023 08:33

sgotti requested changes Dec 14, 2023

View reviewed changes

alessandro-sorint added 2 commits December 14, 2023 10:56

notification: add api to get commit status deliveries

4be43bd

add an api to get commit status deliveries by project id.

gateway: add api to get commit status deliveries

db0b10d

add an api to get commit status deliveries by project ref.

alessandro-sorint force-pushed the api-commitstatusdeliveries branch from e515f66 to db0b10d Compare December 14, 2023 09:57

alessandro-sorint requested a review from sgotti December 14, 2023 09:58

sgotti merged commit 96ec249 into agola-io:master Dec 14, 2023
1 check was pending

alessandro-sorint deleted the api-commitstatusdeliveries branch December 14, 2023 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gateway: add api to get commit status deliveries #460

gateway: add api to get commit status deliveries #460

alessandro-sorint commented Dec 11, 2023

sgotti Dec 12, 2023

sgotti Dec 12, 2023

sgotti Dec 12, 2023

sgotti Dec 12, 2023

sgotti Dec 12, 2023

sgotti Dec 12, 2023

sgotti Dec 13, 2023

sgotti Dec 13, 2023

sgotti Dec 13, 2023

alessandro-sorint Dec 13, 2023

sgotti Dec 13, 2023

sgotti Dec 13, 2023

sgotti Dec 14, 2023

alessandro-sorint Dec 14, 2023

sgotti Dec 14, 2023

alessandro-sorint Dec 14, 2023


		giteaRepo, project := createProject(ctx, t, giteaClient, gwUser01Client, withVisibility(gwapitypes.VisibilityPrivate))

		push(t, config, giteaRepo.CloneURL, giteaToken, "commit", false)


		ns := setupNotificationService(ctx, t, log, dir)

		t.Logf("starting ns")


		giteaRepo, project := createProject(ctx, t, giteaClient, gwUser01Client, withVisibility(gwapitypes.VisibilityPrivate))

		runNumbers := 5

gateway: add api to get commit status deliveries #460

gateway: add api to get commit status deliveries #460

Conversation

alessandro-sorint commented Dec 11, 2023

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