From 832b2b118df8f1714c87bfe48eebf0cbf9848897 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Wed, 24 Jun 2026 20:54:01 -0400 Subject: [PATCH] fix(release): require --sha only for tag-creating manage-release actions manage-release validated --sha as required for every action, but lock, update, and delete resolve the release by tag (SHA is only an optional disambiguator); only create, prerelease, and publish tag a commit and genuinely need it. The over-strict check made the generator's emitted 'manage-release --action delete --sha ""' always error (masked by '|| true'), and broke a lock verb on the fleet. Gate the requirement to the tag-creating actions. Signed-off-by: Joshua Temple --- internal/release/command.go | 45 ++++++++--- internal/release/command_test.go | 125 +++++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+), 12 deletions(-) create mode 100644 internal/release/command_test.go diff --git a/internal/release/command.go b/internal/release/command.go index 0efacdfa..76c1d4bf 100644 --- a/internal/release/command.go +++ b/internal/release/command.go @@ -58,17 +58,8 @@ Outputs (to stdout): } // Validate required fields - if repo == "" { - return fmt.Errorf("--repo is required") - } - if environment == "" { - return fmt.Errorf("--environment is required") - } - if sha == "" { - return fmt.Errorf("--sha is required") - } - if tag == "" { - return fmt.Errorf("--tag is required") + if err := validateManageReleaseFlags(act, repo, environment, sha, tag); err != nil { + return err } // Get token from flag or environment @@ -132,8 +123,38 @@ Outputs (to stdout): _ = cmd.MarkFlagRequired("repo") _ = cmd.MarkFlagRequired("action") _ = cmd.MarkFlagRequired("environment") - _ = cmd.MarkFlagRequired("sha") _ = cmd.MarkFlagRequired("tag") return cmd } + +// tagCreatingActions are the actions that materialize a git tag pointing at a +// specific commit and therefore require --sha. The remaining actions (lock, +// update, delete) resolve an existing release by its tag and treat SHA only as +// an optional disambiguator, so SHA is not required for them. +var tagCreatingActions = map[Action]bool{ + ActionCreate: true, + ActionPrerelease: true, + ActionPublish: true, +} + +// validateManageReleaseFlags checks the flags required by the manage-release +// command. repo, environment, and tag are required for every action. SHA is +// required only for the tag-creating actions (create, prerelease, publish), +// which tag a commit; the tag-addressed actions (lock, update, delete) resolve +// the release by tag and do not need it. +func validateManageReleaseFlags(act Action, repo, environment, sha, tag string) error { + if repo == "" { + return fmt.Errorf("--repo is required") + } + if environment == "" { + return fmt.Errorf("--environment is required") + } + if tag == "" { + return fmt.Errorf("--tag is required") + } + if sha == "" && tagCreatingActions[act] { + return fmt.Errorf("--sha is required for action %q", act) + } + return nil +} diff --git a/internal/release/command_test.go b/internal/release/command_test.go new file mode 100644 index 00000000..bdd10c60 --- /dev/null +++ b/internal/release/command_test.go @@ -0,0 +1,125 @@ +package release + +import ( + "strings" + "testing" +) + +// TestValidateManageReleaseFlags_SHARequiredOnlyForTagCreatingActions asserts +// that --sha is required only for the tag-creating actions (create, prerelease, +// publish) and is optional for the tag-addressed actions (lock, update, delete), +// which resolve the release by tag and treat SHA as an optional disambiguator. +func TestValidateManageReleaseFlags_SHARequiredOnlyForTagCreatingActions(t *testing.T) { + const ( + repo = "owner/repo" + env = "staging" + tag = "v1.0.0-rc.1" + ) + + tests := []struct { + name string + action Action + sha string + wantErr bool + errSubstr string + }{ + { + name: "lock without sha passes", + action: ActionLock, + sha: "", + wantErr: false, + }, + { + name: "update without sha passes", + action: ActionUpdate, + sha: "", + wantErr: false, + }, + { + name: "delete without sha passes", + action: ActionDelete, + sha: "", + wantErr: false, + }, + { + name: "create without sha fails", + action: ActionCreate, + sha: "", + wantErr: true, + errSubstr: "--sha is required", + }, + { + name: "prerelease without sha fails", + action: ActionPrerelease, + sha: "", + wantErr: true, + errSubstr: "--sha is required", + }, + { + name: "publish without sha fails", + action: ActionPublish, + sha: "", + wantErr: true, + errSubstr: "--sha is required", + }, + { + name: "create with sha passes", + action: ActionCreate, + sha: "abc123", + wantErr: false, + }, + { + name: "lock with sha passes", + action: ActionLock, + sha: "abc123", + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateManageReleaseFlags(tt.action, repo, env, tt.sha, tag) + if tt.wantErr { + if err == nil { + t.Fatalf("expected error for action %q with sha=%q, got nil", tt.action, tt.sha) + } + if tt.errSubstr != "" && !strings.Contains(err.Error(), tt.errSubstr) { + t.Fatalf("expected error containing %q, got %q", tt.errSubstr, err.Error()) + } + return + } + if err != nil { + t.Fatalf("expected no error for action %q with sha=%q, got %q", tt.action, tt.sha, err.Error()) + } + }) + } +} + +// TestValidateManageReleaseFlags_OtherRequiredFields confirms repo, environment, +// and tag remain required for every action regardless of the SHA gating. +func TestValidateManageReleaseFlags_OtherRequiredFields(t *testing.T) { + tests := []struct { + name string + repo string + env string + tag string + errSubstr string + }{ + {name: "missing repo", repo: "", env: "staging", tag: "v1", errSubstr: "--repo is required"}, + {name: "missing environment", repo: "owner/repo", env: "", tag: "v1", errSubstr: "--environment is required"}, + {name: "missing tag", repo: "owner/repo", env: "staging", tag: "", errSubstr: "--tag is required"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Use ActionLock (sha-optional) so the failing field is the one under test. + err := validateManageReleaseFlags(ActionLock, tt.repo, tt.env, "", tt.tag) + if err == nil { + t.Fatalf("expected error, got nil") + } + if !strings.Contains(err.Error(), tt.errSubstr) { + t.Fatalf("expected error containing %q, got %q", tt.errSubstr, err.Error()) + } + }) + } +}