From fdb1874ada8b6fae21deb484b13cee8ae4820cff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emmanuel=20BENO=C3=8ET?= Date: Mon, 22 Jul 2024 07:33:45 +0000 Subject: [PATCH] feat(cli): add `--keep-labels` flag to `forgejo actions register` (#4610) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds a new flag, `--keep-labels`, to the runner registration CLI command. If this flag is present and the runner being registered already exists, it will prevent the runners' labels from being reset. In order to accomplish this, the signature of the `RegisterRunner` function from the `models/actions` package has been modified so that the labels argument can be nil. If it is, the part of the function that updates the record will not change the runner. Various tests have been added for this function, for the following cases: new runner with labels, new runner without label, existing runner with labels, existing runner without labels. The flag has been added to the CLI command, the action function has been updated to read the labels parameters through a separate function (`getLabels`), and test cases for this function have been added. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4610 Reviewed-by: Earl Warren Co-authored-by: Emmanuel BENOÎT Co-committed-by: Emmanuel BENOÎT --- cmd/forgejo/actions.go | 23 +++++- cmd/forgejo/actions_test.go | 88 +++++++++++++++++++++ models/actions/forgejo.go | 17 +++-- models/actions/forgejo_test.go | 122 +++++++++++++++++++++++++++++- models/fixtures/action_runner.yml | 2 +- 5 files changed, 241 insertions(+), 11 deletions(-) create mode 100644 cmd/forgejo/actions_test.go diff --git a/cmd/forgejo/actions.go b/cmd/forgejo/actions.go index 70f9452cb..1560b10fa 100644 --- a/cmd/forgejo/actions.go +++ b/cmd/forgejo/actions.go @@ -86,6 +86,11 @@ func SubcmdActionsRegister(ctx context.Context) *cli.Command { Value: "", Usage: "comma separated list of labels supported by the runner (e.g. docker,ubuntu-latest,self-hosted) (not required since v1.21)", }, + &cli.BoolFlag{ + Name: "keep-labels", + Value: false, + Usage: "do not affect the labels when updating an existing runner", + }, &cli.StringFlag{ Name: "name", Value: "runner", @@ -133,6 +138,17 @@ func validateSecret(secret string) error { return nil } +func getLabels(cliCtx *cli.Context) (*[]string, error) { + if !cliCtx.Bool("keep-labels") { + lblValue := strings.Split(cliCtx.String("labels"), ",") + return &lblValue, nil + } + if cliCtx.String("labels") != "" { + return nil, fmt.Errorf("--labels and --keep-labels should not be used together") + } + return nil, nil +} + func RunRegister(ctx context.Context, cliCtx *cli.Context) error { var cancel context.CancelFunc if !ContextGetNoInit(ctx) { @@ -153,9 +169,12 @@ func RunRegister(ctx context.Context, cliCtx *cli.Context) error { return err } scope := cliCtx.String("scope") - labels := cliCtx.String("labels") name := cliCtx.String("name") version := cliCtx.String("version") + labels, err := getLabels(cliCtx) + if err != nil { + return err + } // // There are two kinds of tokens @@ -179,7 +198,7 @@ func RunRegister(ctx context.Context, cliCtx *cli.Context) error { return err } - runner, err := actions_model.RegisterRunner(ctx, owner, repo, secret, strings.Split(labels, ","), name, version) + runner, err := actions_model.RegisterRunner(ctx, owner, repo, secret, labels, name, version) if err != nil { return fmt.Errorf("error while registering runner: %v", err) } diff --git a/cmd/forgejo/actions_test.go b/cmd/forgejo/actions_test.go new file mode 100644 index 000000000..9a9edb7eb --- /dev/null +++ b/cmd/forgejo/actions_test.go @@ -0,0 +1,88 @@ +// Copyright The Forgejo Authors. +// SPDX-License-Identifier: MIT + +package forgejo + +import ( + "fmt" + "testing" + + "code.gitea.io/gitea/services/context" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/urfave/cli/v2" +) + +func TestActions_getLabels(t *testing.T) { + type testCase struct { + args []string + hasLabels bool + hasError bool + labels []string + } + type resultType struct { + labels *[]string + err error + } + + cases := []testCase{ + { + args: []string{"x"}, + hasLabels: true, + hasError: false, + labels: []string{""}, + }, { + args: []string{"x", "--labels", "a,b"}, + hasLabels: true, + hasError: false, + labels: []string{"a", "b"}, + }, { + args: []string{"x", "--keep-labels"}, + hasLabels: false, + hasError: false, + }, { + args: []string{"x", "--keep-labels", "--labels", "a,b"}, + hasLabels: false, + hasError: true, + }, { + // this edge-case exists because that's what actually happens + // when no '--labels ...' options are present + args: []string{"x", "--keep-labels", "--labels", ""}, + hasLabels: false, + hasError: false, + }, + } + + flags := SubcmdActionsRegister(context.Context{}).Flags + for _, c := range cases { + t.Run(fmt.Sprintf("args: %v", c.args), func(t *testing.T) { + // Create a copy of command to test + var result *resultType + app := cli.NewApp() + app.Flags = flags + app.Action = func(ctx *cli.Context) error { + labels, err := getLabels(ctx) + result = &resultType{labels, err} + return nil + } + + // Run it + _ = app.Run(c.args) + + // Test the results + require.NotNil(t, result) + if c.hasLabels { + assert.NotNil(t, result.labels) + assert.Equal(t, c.labels, *result.labels) + } else { + assert.Nil(t, result.labels) + } + if c.hasError { + assert.NotNil(t, result.err) + } else { + assert.Nil(t, result.err) + } + }) + } +} diff --git a/models/actions/forgejo.go b/models/actions/forgejo.go index 243262fac..29e8588e2 100644 --- a/models/actions/forgejo.go +++ b/models/actions/forgejo.go @@ -14,7 +14,7 @@ import ( gouuid "github.com/google/uuid" ) -func RegisterRunner(ctx context.Context, ownerID, repoID int64, token string, labels []string, name, version string) (*ActionRunner, error) { +func RegisterRunner(ctx context.Context, ownerID, repoID int64, token string, labels *[]string, name, version string) (*ActionRunner, error) { uuid, err := gouuid.FromBytes([]byte(token[:16])) if err != nil { return nil, fmt.Errorf("gouuid.FromBytes %v", err) @@ -39,9 +39,10 @@ func RegisterRunner(ctx context.Context, ownerID, repoID int64, token string, la hash := auth_model.HashToken(token, salt) runner = ActionRunner{ - UUID: uuidString, - TokenHash: hash, - TokenSalt: salt, + UUID: uuidString, + TokenHash: hash, + TokenSalt: salt, + AgentLabels: []string{}, } if err := CreateRunner(ctx, &runner); err != nil { @@ -54,13 +55,17 @@ func RegisterRunner(ctx context.Context, ownerID, repoID int64, token string, la // name, _ = util.SplitStringAtByteN(name, 255) + cols := []string{"name", "owner_id", "repo_id", "version"} runner.Name = name runner.OwnerID = ownerID runner.RepoID = repoID runner.Version = version - runner.AgentLabels = labels + if labels != nil { + runner.AgentLabels = *labels + cols = append(cols, "agent_labels") + } - if err := UpdateRunner(ctx, &runner, "name", "owner_id", "repo_id", "version", "agent_labels"); err != nil { + if err := UpdateRunner(ctx, &runner, cols...); err != nil { return &runner, fmt.Errorf("can't update the runner %+v %w", runner, err) } diff --git a/models/actions/forgejo_test.go b/models/actions/forgejo_test.go index a8583c3d0..1bc81e713 100644 --- a/models/actions/forgejo_test.go +++ b/models/actions/forgejo_test.go @@ -13,7 +13,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestActions_RegisterRunner(t *testing.T) { +func TestActions_RegisterRunner_Token(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) ownerID := int64(0) repoID := int64(0) @@ -21,9 +21,127 @@ func TestActions_RegisterRunner(t *testing.T) { labels := []string{} name := "runner" version := "v1.2.3" - runner, err := RegisterRunner(db.DefaultContext, ownerID, repoID, token, labels, name, version) + runner, err := RegisterRunner(db.DefaultContext, ownerID, repoID, token, &labels, name, version) assert.NoError(t, err) assert.EqualValues(t, name, runner.Name) assert.EqualValues(t, 1, subtle.ConstantTimeCompare([]byte(runner.TokenHash), []byte(auth_model.HashToken(token, runner.TokenSalt))), "the token cannot be verified with the same method as routers/api/actions/runner/interceptor.go as of 8228751c55d6a4263f0fec2932ca16181c09c97d") } + +func TestActions_RegisterRunner_CreateWithLabels(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + ownerID := int64(0) + repoID := int64(0) + token := "0123456789012345678901234567890123456789" + name := "runner" + version := "v1.2.3" + labels := []string{"woop", "doop"} + labelsCopy := labels // labels may be affected by the tested function so we copy them + + runner, err := RegisterRunner(db.DefaultContext, ownerID, repoID, token, &labels, name, version) + assert.NoError(t, err) + + // Check that the returned record has been updated, except for the labels + assert.EqualValues(t, ownerID, runner.OwnerID) + assert.EqualValues(t, repoID, runner.RepoID) + assert.EqualValues(t, name, runner.Name) + assert.EqualValues(t, version, runner.Version) + assert.EqualValues(t, labelsCopy, runner.AgentLabels) + + // Check that whatever is in the DB has been updated, except for the labels + after := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: runner.ID}) + assert.EqualValues(t, ownerID, after.OwnerID) + assert.EqualValues(t, repoID, after.RepoID) + assert.EqualValues(t, name, after.Name) + assert.EqualValues(t, version, after.Version) + assert.EqualValues(t, labelsCopy, after.AgentLabels) +} + +func TestActions_RegisterRunner_CreateWithoutLabels(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + ownerID := int64(0) + repoID := int64(0) + token := "0123456789012345678901234567890123456789" + name := "runner" + version := "v1.2.3" + + runner, err := RegisterRunner(db.DefaultContext, ownerID, repoID, token, nil, name, version) + assert.NoError(t, err) + + // Check that the returned record has been updated, except for the labels + assert.EqualValues(t, ownerID, runner.OwnerID) + assert.EqualValues(t, repoID, runner.RepoID) + assert.EqualValues(t, name, runner.Name) + assert.EqualValues(t, version, runner.Version) + assert.EqualValues(t, []string{}, runner.AgentLabels) + + // Check that whatever is in the DB has been updated, except for the labels + after := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: runner.ID}) + assert.EqualValues(t, ownerID, after.OwnerID) + assert.EqualValues(t, repoID, after.RepoID) + assert.EqualValues(t, name, after.Name) + assert.EqualValues(t, version, after.Version) + assert.EqualValues(t, []string{}, after.AgentLabels) +} + +func TestActions_RegisterRunner_UpdateWithLabels(t *testing.T) { + const recordID = 12345678 + token := "7e577e577e577e57feedfacefeedfacefeedface" + assert.NoError(t, unittest.PrepareTestDatabase()) + unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: recordID}) + + newOwnerID := int64(1) + newRepoID := int64(1) + newName := "rennur" + newVersion := "v4.5.6" + newLabels := []string{"warp", "darp"} + labelsCopy := newLabels // labels may be affected by the tested function so we copy them + + runner, err := RegisterRunner(db.DefaultContext, newOwnerID, newRepoID, token, &newLabels, newName, newVersion) + assert.NoError(t, err) + + // Check that the returned record has been updated + assert.EqualValues(t, newOwnerID, runner.OwnerID) + assert.EqualValues(t, newRepoID, runner.RepoID) + assert.EqualValues(t, newName, runner.Name) + assert.EqualValues(t, newVersion, runner.Version) + assert.EqualValues(t, labelsCopy, runner.AgentLabels) + + // Check that whatever is in the DB has been updated + after := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: recordID}) + assert.EqualValues(t, newOwnerID, after.OwnerID) + assert.EqualValues(t, newRepoID, after.RepoID) + assert.EqualValues(t, newName, after.Name) + assert.EqualValues(t, newVersion, after.Version) + assert.EqualValues(t, labelsCopy, after.AgentLabels) +} + +func TestActions_RegisterRunner_UpdateWithoutLabels(t *testing.T) { + const recordID = 12345678 + token := "7e577e577e577e57feedfacefeedfacefeedface" + assert.NoError(t, unittest.PrepareTestDatabase()) + before := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: recordID}) + + newOwnerID := int64(1) + newRepoID := int64(1) + newName := "rennur" + newVersion := "v4.5.6" + + runner, err := RegisterRunner(db.DefaultContext, newOwnerID, newRepoID, token, nil, newName, newVersion) + assert.NoError(t, err) + + // Check that the returned record has been updated, except for the labels + assert.EqualValues(t, newOwnerID, runner.OwnerID) + assert.EqualValues(t, newRepoID, runner.RepoID) + assert.EqualValues(t, newName, runner.Name) + assert.EqualValues(t, newVersion, runner.Version) + assert.EqualValues(t, before.AgentLabels, runner.AgentLabels) + + // Check that whatever is in the DB has been updated, except for the labels + after := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: recordID}) + assert.EqualValues(t, newOwnerID, after.OwnerID) + assert.EqualValues(t, newRepoID, after.RepoID) + assert.EqualValues(t, newName, after.Name) + assert.EqualValues(t, newVersion, after.Version) + assert.EqualValues(t, before.AgentLabels, after.AgentLabels) +} diff --git a/models/fixtures/action_runner.yml b/models/fixtures/action_runner.yml index d2615f08e..94deac998 100644 --- a/models/fixtures/action_runner.yml +++ b/models/fixtures/action_runner.yml @@ -14,7 +14,7 @@ token_salt: "832f8529db6151a1c3c605dd7570b58f" last_online: 0 last_active: 0 - agent_labels: '[""]' + agent_labels: '["woop", "doop"]' created: 1716104432 updated: 1716104432 deleted: ~