From 7338ebfa7eb19a839d3bd0e490abf574d3d88c9b Mon Sep 17 00:00:00 2001 From: florentianayuwono Date: Fri, 30 Jan 2026 21:54:12 +0700 Subject: [PATCH 1/3] fix: update flavor implementation --- internal/database/database.go | 44 ------------------ internal/database/database_test.go | 61 ------------------------- internal/planner/server.go | 36 ++++++++------- internal/planner/server_test.go | 71 +++++++++++++++++++----------- internal/planner/telemetry_test.go | 14 ++++-- 5 files changed, 75 insertions(+), 151 deletions(-) diff --git a/internal/database/database.go b/internal/database/database.go index 9ae6a4ec..91cd16c6 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -397,50 +397,6 @@ func (d *Database) GetFlavor(ctx context.Context, name string) (*Flavor, error) return &flavor, nil } -// UpdateFlavor updates an existing flavor. -// If the flavor doesn't exist, it will return ErrNotExist. -func (d *Database) UpdateFlavor(ctx context.Context, flavor *Flavor) error { - batch := &pgx.Batch{} - batch.Queue(` - UPDATE flavor - SET - platform = @platform, - labels = @labels, - priority = @priority, - is_disabled = @is_disabled, - minimum_pressure = @minimum_pressure - WHERE name = @name - `, pgx.NamedArgs{ - "platform": flavor.Platform, - "name": flavor.Name, - "labels": flavor.Labels, - "priority": flavor.Priority, - "is_disabled": flavor.IsDisabled, - "minimum_pressure": flavor.MinimumPressure, - }) - batch.Queue(updateAssignedFlavorSql) - batch.Queue(pressureChangeSql) - result := d.conn.SendBatch(ctx, batch) - defer func() { - err := result.Close() - if err != nil { - slog.ErrorContext(ctx, "cannot close batch execution for updating flavors", "error", err) - } - }() - - for i := range batch.Len() { - tag, err := result.Exec() - if err != nil { - return fmt.Errorf("cannot update flavor: %w", err) - } - if i == 0 && tag.RowsAffected() == 0 { - return ErrNotExist - } - } - - return nil -} - func (d *Database) setFlavorIsDisabled(ctx context.Context, platform, name string, isDisabled bool) error { batch := &pgx.Batch{} if isDisabled { diff --git a/internal/database/database_test.go b/internal/database/database_test.go index 750b419a..fa6b37d2 100644 --- a/internal/database/database_test.go +++ b/internal/database/database_test.go @@ -560,67 +560,6 @@ func TestDatabase_GetFlavor_NotExists(t *testing.T) { assert.Nil(t, flavor) } -func TestDatabase_UpdateFlavor(t *testing.T) { - /* - arrange: set up database with a flavor - act: update the flavor - assert: flavor in database is updated - */ - db := setupDatabase(t) - defer teardownDatabase(t) - ctx := t.Context() - - flavor := Flavor{ - Platform: "github", - Name: "amd64-large", - Labels: []string{"self-hosted", "amd64", "large"}, - Priority: 0, - IsDisabled: false, - MinimumPressure: 0, - } - updatedFlavor := Flavor{ - Platform: "github", - Name: "amd64-large", - Labels: []string{"self-hosted", "amd64", "large"}, - Priority: 10, - IsDisabled: true, - MinimumPressure: 5, - } - - assert.NoError(t, db.AddFlavor(ctx, &flavor)) - - pressureUpdates := subscribeToPressureUpdates(t, ctx, db) - assert.NoError(t, db.UpdateFlavor(ctx, &updatedFlavor)) - assertSingleNotificationReceived(t, pressureUpdates) - - flavors, err := db.ListFlavors(ctx, flavor.Platform) - assert.NoError(t, err) - - assert.Len(t, flavors, 1) - assert.Equal(t, updatedFlavor, flavors[0]) -} - -func TestDatabase_UpdateFlavor_NotExists(t *testing.T) { - /* - arrange: set up an empty database - act: update a non-existent flavor - assert: error is raised - */ - db := setupDatabase(t) - defer teardownDatabase(t) - ctx := t.Context() - - notExistingFlavor := Flavor{ - Platform: "github", - Name: "amd64-large", - Labels: []string{"self-hosted", "amd64", "large"}, - Priority: 0, - IsDisabled: false, - MinimumPressure: 0, - } - assert.ErrorIs(t, db.UpdateFlavor(ctx, ¬ExistingFlavor), ErrNotExist) -} - func TestDatabase_DisableFlavor(t *testing.T) { db := setupDatabase(t) defer teardownDatabase(t) diff --git a/internal/planner/server.go b/internal/planner/server.go index 7252f19a..d4abf11e 100644 --- a/internal/planner/server.go +++ b/internal/planner/server.go @@ -45,7 +45,8 @@ var ( type FlavorStore interface { AddFlavor(ctx context.Context, flavor *database.Flavor) error ListFlavors(ctx context.Context, platform string) ([]database.Flavor, error) - UpdateFlavor(ctx context.Context, flavor *database.Flavor) error + EnableFlavor(ctx context.Context, platform, name string) error + DisableFlavor(ctx context.Context, platform, name string) error GetFlavor(ctx context.Context, name string) (*database.Flavor, error) DeleteFlavor(ctx context.Context, platform string, name string) error GetPressures(ctx context.Context, platform string, flavors ...string) (map[string]int, error) @@ -165,14 +166,17 @@ func (s *Server) getFlavor(w http.ResponseWriter, r *http.Request) { respondWithJSON(w, http.StatusOK, flavor) } -// updateFlavor handles updating an existing flavor. -// If the flavor name is allFlavorName, returns status code 400. -// If the flavor is not found, returns status code 404. -// If updating the flavor fails, returns status code 500. -// If successful, returns status code 200. +type updateFlavorRequest struct { + IsDisabled bool `json:"is_disabled"` +} + +// updateFlavor handles updating an existing flavor's disabled status. func (s *Server) updateFlavor(w http.ResponseWriter, r *http.Request) { - req, err := decodeFlavorInRequestBody(r) - if err != nil { + decoder := json.NewDecoder(r.Body) + defer r.Body.Close() + + req := &updateFlavorRequest{} + if err := decoder.Decode(req); err != nil { http.Error(w, fmt.Sprintf("invalid payload: %v", err), http.StatusBadRequest) return } @@ -182,16 +186,14 @@ func (s *Server) updateFlavor(w http.ResponseWriter, r *http.Request) { http.Error(w, "update all flavors is not supported", http.StatusBadRequest) return } - flavor := &database.Flavor{ - Name: flavorName, - Platform: req.Platform, - Labels: req.Labels, - Priority: req.Priority, - IsDisabled: req.IsDisabled, - MinimumPressure: req.MinimumPressure, + + var err error + if req.IsDisabled { + err = s.store.DisableFlavor(r.Context(), flavorPlatform, flavorName) + } else { + err = s.store.EnableFlavor(r.Context(), flavorPlatform, flavorName) } - err = s.store.UpdateFlavor(r.Context(), flavor) if errors.Is(err, database.ErrNotExist) { http.Error(w, "cannot find flavor to update", http.StatusNotFound) return @@ -200,7 +202,7 @@ func (s *Server) updateFlavor(w http.ResponseWriter, r *http.Request) { http.Error(w, fmt.Sprintf("cannot update flavor: %v", err), http.StatusInternalServerError) return } - w.WriteHeader(http.StatusOK) + w.WriteHeader(http.StatusNoContent) } // deleteFlavor handles deleting an existing flavor. diff --git a/internal/planner/server_test.go b/internal/planner/server_test.go index 35ec7fdd..9038908e 100644 --- a/internal/planner/server_test.go +++ b/internal/planner/server_test.go @@ -91,13 +91,22 @@ func (f *fakeStore) GetFlavor(ctx context.Context, name string) (*database.Flavo return nil, f.errToReturn } -func (f *fakeStore) UpdateFlavor(ctx context.Context, flavor *database.Flavor) error { - f.lastFlavor = flavor +func (f *fakeStore) DeleteFlavor(ctx context.Context, platform, name string) error { + f.lastFlavor = nil return f.errToReturn } -func (f *fakeStore) DeleteFlavor(ctx context.Context, platform, name string) error { - f.lastFlavor = nil +func (f *fakeStore) EnableFlavor(ctx context.Context, platform, name string) error { + if f.lastFlavor != nil && f.lastFlavor.Name == name { + f.lastFlavor.IsDisabled = false + } + return f.errToReturn +} + +func (f *fakeStore) DisableFlavor(ctx context.Context, platform, name string) error { + if f.lastFlavor != nil && f.lastFlavor.Name == name { + f.lastFlavor.IsDisabled = true + } return f.errToReturn } @@ -352,15 +361,15 @@ func TestUpdateFlavor(t *testing.T) { assert: Verify the flavors are updated, and/or the HTTP status codes are as expected. */ tests := []struct { - name string - storeFlavor *database.Flavor - storeErr error - url string - body string - expectedStatus int - assertFlavor bool + name string + storeFlavor *database.Flavor + storeErr error + url string + body string + expectedStatus int + expectedDisabled bool }{{ - name: "shouldSucceed", + name: "shouldDisableFlavor", storeFlavor: &database.Flavor{ Platform: "github", Name: "runner-small", @@ -369,24 +378,36 @@ func TestUpdateFlavor(t *testing.T) { IsDisabled: false, MinimumPressure: 5, }, - url: "/api/v1/flavors/runner-small", - body: `{"platform":"github","name":"runner-small","labels":["x64"],"priority":5,"is_disabled":false,"minimum_pressure":10}`, - expectedStatus: http.StatusOK, - assertFlavor: true, + url: "/api/v1/flavors/runner-small", + body: `{"is_disabled":true}`, + expectedStatus: http.StatusNoContent, + expectedDisabled: true, + }, { + name: "shouldEnableFlavor", + storeFlavor: &database.Flavor{ + Platform: "github", + Name: "runner-small", + Labels: []string{"x64"}, + Priority: 10, + IsDisabled: true, + MinimumPressure: 5, + }, + url: "/api/v1/flavors/runner-small", + body: `{"is_disabled":false}`, + expectedStatus: http.StatusNoContent, + expectedDisabled: false, }, { name: "shouldFailWhenFlavorMissing", storeErr: database.ErrNotExist, url: "/api/v1/flavors/not-exist", - body: `{"platform":"github","name":"not-exist","labels":["x64"],"priority":5,"is_disabled":false,"minimum_pressure":10}`, + body: `{"is_disabled":true}`, expectedStatus: http.StatusNotFound, - assertFlavor: false, }, { name: "shouldFailWhenDatabaseError", storeErr: errors.New("database error"), url: "/api/v1/flavors/runner-small", - body: `{"platform":"github","name":"runner-small","labels":["x64"],"priority":5,"is_disabled":false,"minimum_pressure":10}`, + body: `{"is_disabled":false}`, expectedStatus: http.StatusInternalServerError, - assertFlavor: false, }, { name: "shouldFailWhenNameIsAllFlavors", storeFlavor: &database.Flavor{ @@ -398,7 +419,7 @@ func TestUpdateFlavor(t *testing.T) { MinimumPressure: 5, }, url: "/api/v1/flavors/_", - body: `{"platform":"github","name":"not-exist","labels":["x64"],"priority":5,"is_disabled":false,"minimum_pressure":10}`, + body: `{"is_disabled":true}`, expectedStatus: http.StatusBadRequest, }, { name: "shouldFailOnInvalidJSON", @@ -427,12 +448,10 @@ func TestUpdateFlavor(t *testing.T) { server.ServeHTTP(w, req) assert.Equal(t, tt.expectedStatus, w.Code) - if tt.assertFlavor { - flavor := &database.Flavor{} - require.NoError(t, json.Unmarshal([]byte(tt.body), flavor)) - storedFlavor, err := store.GetFlavor(t.Context(), flavor.Name) + if tt.expectedDisabled { + storedFlavor, err := store.GetFlavor(t.Context(), tt.storeFlavor.Name) require.NoError(t, err) - assert.Equal(t, flavor, storedFlavor) + assert.Equal(t, tt.expectedDisabled, storedFlavor.IsDisabled) } }) } diff --git a/internal/planner/telemetry_test.go b/internal/planner/telemetry_test.go index 056bd09c..1e0a3289 100644 --- a/internal/planner/telemetry_test.go +++ b/internal/planner/telemetry_test.go @@ -46,11 +46,19 @@ func (m *mockStore) ListFlavors(ctx context.Context, platform string) ([]databas func (m *mockStore) GetFlavor(ctx context.Context, name string) (*database.Flavor, error) { return m.lastFlavor, nil } -func (m *mockStore) UpdateFlavor(ctx context.Context, flavor *database.Flavor) error { - m.lastFlavor = flavor +func (m *mockStore) DeleteFlavor(ctx context.Context, platform, name string) error { return nil } -func (m *mockStore) DeleteFlavor(ctx context.Context, platform, name string) error { +func (m *mockStore) EnableFlavor(ctx context.Context, platform, name string) error { + if m.lastFlavor != nil && m.lastFlavor.Name == name { + m.lastFlavor.IsDisabled = false + } + return nil +} +func (m *mockStore) DisableFlavor(ctx context.Context, platform, name string) error { + if m.lastFlavor != nil && m.lastFlavor.Name == name { + m.lastFlavor.IsDisabled = true + } return nil } func (m *mockStore) GetPressures(ctx context.Context, platform string, flavors ...string) (map[string]int, error) { From 104cfcc7da555c2dbc5f638e5ca4e03289cc7097 Mon Sep 17 00:00:00 2001 From: florentianayuwono Date: Fri, 30 Jan 2026 22:05:02 +0700 Subject: [PATCH 2/3] address review --- internal/planner/server.go | 9 +++++++-- internal/planner/server_test.go | 15 ++++++++++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/internal/planner/server.go b/internal/planner/server.go index d4abf11e..9230e332 100644 --- a/internal/planner/server.go +++ b/internal/planner/server.go @@ -167,7 +167,7 @@ func (s *Server) getFlavor(w http.ResponseWriter, r *http.Request) { } type updateFlavorRequest struct { - IsDisabled bool `json:"is_disabled"` + IsDisabled *bool `json:"is_disabled"` } // updateFlavor handles updating an existing flavor's disabled status. @@ -181,6 +181,11 @@ func (s *Server) updateFlavor(w http.ResponseWriter, r *http.Request) { return } + if req.IsDisabled == nil { + http.Error(w, "is_disabled field is required", http.StatusBadRequest) + return + } + flavorName := r.PathValue("name") if flavorName == allFlavorName { http.Error(w, "update all flavors is not supported", http.StatusBadRequest) @@ -188,7 +193,7 @@ func (s *Server) updateFlavor(w http.ResponseWriter, r *http.Request) { } var err error - if req.IsDisabled { + if *req.IsDisabled { err = s.store.DisableFlavor(r.Context(), flavorPlatform, flavorName) } else { err = s.store.EnableFlavor(r.Context(), flavorPlatform, flavorName) diff --git a/internal/planner/server_test.go b/internal/planner/server_test.go index 9038908e..e20e0134 100644 --- a/internal/planner/server_test.go +++ b/internal/planner/server_test.go @@ -434,6 +434,19 @@ func TestUpdateFlavor(t *testing.T) { url: "/api/v1/flavors/runner-small", body: `{invalid-json}`, expectedStatus: http.StatusBadRequest, + }, { + name: "shouldFailWhenIsDisabledFieldMissing", + storeFlavor: &database.Flavor{ + Platform: "github", + Name: "runner-small", + Labels: []string{"x64"}, + Priority: 10, + IsDisabled: false, + MinimumPressure: 5, + }, + url: "/api/v1/flavors/runner-small", + body: `{}`, + expectedStatus: http.StatusBadRequest, }} for _, tt := range tests { @@ -448,7 +461,7 @@ func TestUpdateFlavor(t *testing.T) { server.ServeHTTP(w, req) assert.Equal(t, tt.expectedStatus, w.Code) - if tt.expectedDisabled { + if tt.expectedStatus == http.StatusNoContent { storedFlavor, err := store.GetFlavor(t.Context(), tt.storeFlavor.Name) require.NoError(t, err) assert.Equal(t, tt.expectedDisabled, storedFlavor.IsDisabled) From eedf91bb106358ae217006e60b71c8d27bd19d4b Mon Sep 17 00:00:00 2001 From: florentianayuwono Date: Mon, 2 Feb 2026 13:41:41 +0700 Subject: [PATCH 3/3] disable test cache --- .github/workflows/internal_tests.yaml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/internal_tests.yaml b/.github/workflows/internal_tests.yaml index e399ab40..eb2c65b6 100644 --- a/.github/workflows/internal_tests.yaml +++ b/.github/workflows/internal_tests.yaml @@ -42,7 +42,7 @@ jobs: # We exclude packages from coverage analysis as they are highly uncovered at the moment EXCLUDE_PKGS="internal/telemetry" COVER_PKG=$(go list ./internal/... | grep -Ev "$EXCLUDE_PKGS" | tr '\n' ',') - go test -v -coverprofile=coverage.out -race -tags=integration -coverpkg=$COVER_PKG ./internal/... + go test -v -count=1 -coverprofile=coverage.out -race -tags=integration -coverpkg=$COVER_PKG ./internal/... - name: Print coverage report run: | @@ -63,7 +63,6 @@ jobs: exit 1 fi - services: postgres: image: postgres:16.11