tech qa feedback, add test for update & fix issue with visiability & config

core/cancel.go

@@ -21,7 +21,7 @@ type Canceler interface {
 	// Cancel cancels the provided build.
 	Cancel(context.Context, *Repository, *Build) error
 
-	// CancelByStatus cancels all builds by a status, passed to the function
-	// and reference with lower build numbers.
+	// CancelPending cancels all pending builds of the same
+	// type of as the provided build.
 	CancelPending(context.Context, *Repository, *Build) error
 }

handler/api/repos/update.go

@@ -28,8 +28,8 @@ import (
 
 type (
 	repositoryInput struct {
-		Visibility    *string `json:"visibility"`
-		Config        *string `json:"config_path"`
+		Visibility    string `json:"visibility"`
+		Config        string `json:"config_path"`
 		Trusted       *bool  `json:"trusted"`
 		Protected     *bool  `json:"protected"`
 		IgnoreForks   *bool  `json:"ignore_forks"`
@@ -75,11 +75,11 @@ func HandleUpdate(repos core.RepositoryStore) http.HandlerFunc {
 			return
 		}
 
-		if in.Visibility != nil {
-			repo.Visibility = *in.Visibility
+		if in.Visibility != "" {
+			repo.Visibility = in.Visibility
 		}
-		if in.Config != nil {
-			repo.Config = *in.Config
+		if in.Config != "" {
+			repo.Config = in.Config
 		}
 		if in.Protected != nil {
 			repo.Protected = *in.Protected

handler/api/repos/update_test.go

@@ -41,7 +41,6 @@ func TestUpdate(t *testing.T) {
 
 	repoInput := &core.Repository{
 		Visibility: core.VisibilityPublic,
-		CancelRunning: true,
 	}
 
 	checkUpdate := func(_ context.Context, updated *core.Repository) error {
@@ -84,7 +83,6 @@ func TestUpdate(t *testing.T) {
 		HTTPURL:    "https://github.com/octocat/hello-world.git",
 		SSHURL:     "git@github.com:octocat/hello-world.git",
 		Link:       "https://github.com/octocat/hello-world",
-		CancelRunning: true,
 	}
 	json.NewDecoder(w.Body).Decode(got)
 	if diff := cmp.Diff(got, want); len(diff) > 0 {
@@ -222,3 +220,75 @@ func TestUpdate_UpdateFailed(t *testing.T) {
 		t.Errorf(diff)
 	}
 }
+
+func TestUpdateAutoCancelRunning(t *testing.T) {
+	controller := gomock.NewController(t)
+	defer controller.Finish()
+
+	repo := &core.Repository{
+		ID:         1,
+		UserID:     1,
+		Namespace:  "octocat",
+		Name:       "hello-world",
+		Slug:       "octocat/hello-world",
+		Branch:     "master",
+		Private:    false,
+		Visibility: core.VisibilityPrivate,
+		HTTPURL:    "https://github.com/octocat/hello-world.git",
+		SSHURL:     "git@github.com:octocat/hello-world.git",
+		Link:       "https://github.com/octocat/hello-world",
+		CancelRunning: false,
+	}
+
+	repoInput := &core.Repository{
+		CancelRunning: true,
+	}
+
+	shouldBeValue := true
+	checkUpdate := func(_ context.Context, updated *core.Repository) error {
+		if got, want := updated.CancelRunning, shouldBeValue; got != want {
+			t.Errorf("Want repository visibility updated to %v, got %v", want, got)
+		}
+		return nil
+	}
+
+	repos := mock.NewMockRepositoryStore(controller)
+	repos.EXPECT().FindName(gomock.Any(), "octocat", "hello-world").Return(repo, nil)
+	repos.EXPECT().Update(gomock.Any(), repo).Return(nil).Do(checkUpdate)
+
+	c := new(chi.Context)
+	c.URLParams.Add("owner", "octocat")
+	c.URLParams.Add("name", "hello-world")
+
+	in := new(bytes.Buffer)
+	json.NewEncoder(in).Encode(repoInput)
+	w := httptest.NewRecorder()
+	r := httptest.NewRequest("POST", "/", in)
+	r = r.WithContext(
+		context.WithValue(r.Context(), chi.RouteCtxKey, c),
+	)
+
+	HandleUpdate(repos)(w, r)
+	if got, want := w.Code, 200; want != got {
+		t.Errorf("Want response code %d, got %d", want, got)
+	}
+
+	got, want := new(core.Repository), &core.Repository{
+		ID:         1,
+		UserID:     1,
+		Namespace:  "octocat",
+		Name:       "hello-world",
+		Slug:       "octocat/hello-world",
+		Branch:     "master",
+		Private:    false,
+		Visibility: core.VisibilityPrivate,
+		HTTPURL:    "https://github.com/octocat/hello-world.git",
+		SSHURL:     "git@github.com:octocat/hello-world.git",
+		Link:       "https://github.com/octocat/hello-world",
+		CancelRunning: true,
+	}
+	json.NewDecoder(w.Body).Decode(got)
+	if diff := cmp.Diff(got, want); len(diff) > 0 {
+		t.Errorf(diff)
+	}
+}

service/canceler/canceler.go

@@ -105,9 +105,7 @@ func (s *service) CancelPending(ctx context.Context, repo *core.Repository, buil
 	var result error
 	for _, item := range incomplete {
 		// ignore incomplete items in the list that do
-		// not match the repository or build, are already
-		// running, or are newer than the current build.
-
+		// not match the repository or build
 		if !match(build, item) {
 			continue
 		}