From f5426c9d1e79cb985fb4dddce27e1526f427172c Mon Sep 17 00:00:00 2001 From: tuti Date: Fri, 20 Feb 2026 21:46:01 -0800 Subject: [PATCH 1/8] feat(release): implement extensible hook system with timeout add a generic, extensible hook system to the release tooling that allows external code to extend build and publish workflows without modifying core logic. --- hack/release/build.go | 116 ++++++++++----- hack/release/checks.go | 2 +- hack/release/flags.go | 35 ++++- hack/release/hooks.go | 233 +++++++++++++++++++++++++++++ hack/release/hooks_test.go | 293 +++++++++++++++++++++++++++++++++++++ hack/release/prep.go | 2 +- hack/release/publish.go | 42 +++++- hack/release/utils.go | 44 ++++++ hack/release/utils_test.go | 55 +++++++ 9 files changed, 778 insertions(+), 44 deletions(-) create mode 100644 hack/release/hooks.go create mode 100644 hack/release/hooks_test.go diff --git a/hack/release/build.go b/hack/release/build.go index 1057b5ee7f..ea6d31f7da 100644 --- a/hack/release/build.go +++ b/hack/release/build.go @@ -85,15 +85,38 @@ var buildCommand = &cli.Command{ enterpriseDirFlag, hashreleaseFlag, skipValidationFlag, + hookTimeoutFlag, }, Before: buildBefore, Action: buildAction, } +// buildBeforeHook is an optional hook called at the start of buildBefore for additional pre-processing. +// It can be set via init() in separate files to extend the build command behavior. +var buildBeforeHook cliBeforeHookFunc + +// setupHashreleasePreHook is an optional hook called at the start of setupHashreleaseBuild +// for additional hashrelease setup (e.g., copying image config files). +var setupHashreleasePreHook cliHookWithRepoDirFunc + +// buildAfterHook is an optional hook called in buildAfter for additional post-build tasks. +// This run regardless of build success or failure, so it can be used for cleanup tasks. +var buildAfterHooks multiHook + // Pre-action for release build command. var buildBefore = cli.BeforeFunc(func(ctx context.Context, c *cli.Command) (context.Context, error) { configureLogging(c) + // Extract hook timeout once for reuse + hookTimeout := c.Duration(hookTimeoutFlag.Name) + + // Call pre-hook if registered. + var err error + ctx, err = RunBuildBeforeHook(ctx, c, hookTimeout) + if err != nil { + return ctx, err + } + // Determine build types for Calico and Enterprise if ver := c.String(calicoVersionsConfigFlag.Name); ver != "" { ctx = context.WithValue(ctx, calicoBuildCtxKey, versionsBuild) @@ -112,8 +135,8 @@ var buildBefore = cli.BeforeFunc(func(ctx context.Context, c *cli.Command) (cont logrus.Debug("Enterprise build using specific version selected") } - // Run verison validations. This is a mandatory check. - ctx, err := checkVersion(ctx, c) + // Run version validations. This is a mandatory check. + ctx, err = checkVersion(ctx, c) if err != nil { return ctx, err } @@ -174,6 +197,16 @@ var buildAction = cli.ActionFunc(func(ctx context.Context, c *cli.Command) error version := c.String(versionFlag.Name) buildLog := logrus.WithField("version", version) + // For hashrelease builds, skip if image is already published. + if c.Bool(hashreleaseFlag.Name) { + if published, err := operatorImagePublished(c); err != nil { + buildLog.WithError(err).Warn("Failed to check if image is already published, proceeding with build") + } else if published { + buildLog.Warn("Image is already published, skipping build") + return nil + } + } + // Prepare build environment variables buildEnv := append(os.Environ(), fmt.Sprintf("VERSION=%s", version)) if arches := c.StringSlice(archFlag.Name); len(arches) > 0 { @@ -197,11 +230,7 @@ var buildAction = cli.ActionFunc(func(ctx context.Context, c *cli.Command) error if c.Bool(hashreleaseFlag.Name) { buildLog = buildLog.WithField("hashrelease", true) buildEnv = append(buildEnv, fmt.Sprintf("GIT_VERSION=%s", c.String(versionFlag.Name))) - resetFn, err := setupHashreleaseBuild(ctx, c, repoRootDir) - // Ensure git state is reset after build. - // If there was an error preparing the build, reset any partial changes first before returning the error. - defer resetFn() - if err != nil { + if err := setupHashreleaseBuild(ctx, c, repoRootDir); err != nil { return fmt.Errorf("preparing hashrelease build environment: %w", err) } } else { @@ -222,6 +251,15 @@ var buildAction = cli.ActionFunc(func(ctx context.Context, c *cli.Command) error return nil }) +var buildAfter = cli.AfterFunc(func(ctx context.Context, c *cli.Command) error { + hookTimeout := c.Duration(hookTimeoutFlag.Name) + if err := RunBuildAfterHook(ctx, c, hookTimeout); err != nil { + // Error should not fail the build, but log it for visibility. + logrus.WithError(err).Error("Build after hook failed") + } + return nil +}) + // List images in the built operator image for debugging purposes. func listImages(registry, image, version string) { fqImage := fmt.Sprintf("%s:%s-%s", path.Join(registry, image), version, runtime.GOARCH) @@ -257,45 +295,55 @@ func assertOperatorImageVersion(registry, image, expectedVersion string) error { return nil } -// Modify component images config and versions for hashrelease builds as needed. -// Returns a function to reset the git state and any error encountered. -func setupHashreleaseBuild(ctx context.Context, c *cli.Command, repoRootDir string) (func(), error) { - repoReset := func() { - if out, err := gitInDir(repoRootDir, append([]string{"checkout", "-f"}, changedFiles...)...); err != nil { - logrus.WithError(err).Errorf("resetting git state: %s", out) +// Modify component images config and versions for hashrelease builds as needed +// and adds a build after hook to reset any changes to git state after the build completes. +func setupHashreleaseBuild(ctx context.Context, c *cli.Command, repoRootDir string) error { + buildAfterHooks.Add("Reset repo git state", func(ctx context.Context, c *cli.Command) error { + if out, err := gitInDirContext(ctx, repoRootDir, append([]string{"checkout", "-f"}, changedFiles...)...); err != nil { + logrus.Error(out) + return fmt.Errorf("resetting git state in repo after hashrelease build: %w", err) } + return nil + }) + + // Call pre-hook if registered. + var err error + hookTimeout := c.Duration(hookTimeoutFlag.Name) + if ctx, err = RunSetupHashreleasePreHook(ctx, c, repoRootDir, hookTimeout); err != nil { + return fmt.Errorf("setup hashrelease pre-hook failed: %w", err) } + image := c.String(imageFlag.Name) if image != defaultImageName { imageParts := strings.SplitN(c.String(imageFlag.Name), "/", 2) - if err := modifyComponentImageConfig(repoRootDir, operatorImagePathConfigKey, addTrailingSlash(imageParts[0])); err != nil { - return repoReset, fmt.Errorf("updating Operator image path: %w", err) + if err := modifyComponentImageConfig(repoRootDir, componentImageConfigRelPath, operatorImagePathConfigKey, addTrailingSlash(imageParts[0])); err != nil { + return fmt.Errorf("updating Operator image path: %w", err) } } registry := c.String(registryFlag.Name) if registry != "" && registry != quayRegistry { - if err := modifyComponentImageConfig(repoRootDir, operatorRegistryConfigKey, addTrailingSlash(registry)); err != nil { - return repoReset, fmt.Errorf("updating Operator registry: %w", err) + if err := modifyComponentImageConfig(repoRootDir, componentImageConfigRelPath, operatorRegistryConfigKey, addTrailingSlash(registry)); err != nil { + return fmt.Errorf("updating Operator registry: %w", err) } } if registry := c.String(calicoRegistryFlag.Name); registry != "" { - if err := modifyComponentImageConfig(repoRootDir, calicoRegistryConfigKey, addTrailingSlash(registry)); err != nil { - return repoReset, fmt.Errorf("updating Calico registry: %w", err) + if err := modifyComponentImageConfig(repoRootDir, componentImageConfigRelPath, calicoRegistryConfigKey, addTrailingSlash(registry)); err != nil { + return fmt.Errorf("updating Calico registry: %w", err) } } if imagePath := c.String(calicoImagePathFlag.Name); imagePath != "" { - if err := modifyComponentImageConfig(repoRootDir, calicoImagePathConfigKey, imagePath); err != nil { - return repoReset, fmt.Errorf("updating Calico image path: %w", err) + if err := modifyComponentImageConfig(repoRootDir, componentImageConfigRelPath, calicoImagePathConfigKey, imagePath); err != nil { + return fmt.Errorf("updating Calico image path: %w", err) } } if registry := c.String(enterpriseRegistryFlag.Name); registry != "" { - if err := modifyComponentImageConfig(repoRootDir, enterpriseRegistryConfigKey, addTrailingSlash(registry)); err != nil { - return repoReset, fmt.Errorf("updating Enterprise registry: %w", err) + if err := modifyComponentImageConfig(repoRootDir, componentImageConfigRelPath, enterpriseRegistryConfigKey, addTrailingSlash(registry)); err != nil { + return fmt.Errorf("updating Enterprise registry: %w", err) } } if imagePath := c.String(enterpriseImagePathFlag.Name); imagePath != "" { - if err := modifyComponentImageConfig(repoRootDir, enterpriseImagePathConfigKey, imagePath); err != nil { - return repoReset, fmt.Errorf("updating Enterprise image path: %w", err) + if err := modifyComponentImageConfig(repoRootDir, componentImageConfigRelPath, enterpriseImagePathConfigKey, imagePath); err != nil { + return fmt.Errorf("updating Enterprise image path: %w", err) } } @@ -313,7 +361,7 @@ func setupHashreleaseBuild(ctx context.Context, c *cli.Command, repoRootDir stri switch bt { case versionBuild: if err := updateConfigVersions(repoRootDir, calicoConfig, c.String(calicoVersionFlag.Name)); err != nil { - return repoReset, fmt.Errorf("updating Calico config versions: %w", err) + return fmt.Errorf("updating Calico config versions: %w", err) } case versionsBuild: genEnv = append(genEnv, fmt.Sprintf("OS_VERSIONS=%s", c.String(calicoVersionsConfigFlag.Name))) @@ -324,7 +372,7 @@ func setupHashreleaseBuild(ctx context.Context, c *cli.Command, repoRootDir stri switch bt { case versionBuild: if err := updateConfigVersions(repoRootDir, enterpriseConfig, c.String(enterpriseVersionFlag.Name)); err != nil { - return repoReset, fmt.Errorf("updating Enterprise config versions: %w", err) + return fmt.Errorf("updating Enterprise config versions: %w", err) } case versionsBuild: genEnv = append(genEnv, fmt.Sprintf("EE_VERSIONS=%s", c.String(enterpriseVersionsConfigFlag.Name))) @@ -332,24 +380,24 @@ func setupHashreleaseBuild(ctx context.Context, c *cli.Command, repoRootDir stri } if out, err := makeInDir(repoRootDir, strings.Join(genMakeTargets, " "), genEnv...); err != nil { logrus.Error(out) - return repoReset, fmt.Errorf("generating versions: %w", err) + return fmt.Errorf("generating versions: %w", err) } - return repoReset, nil + return nil } -// Modify variables in pkg/components/images.go -func modifyComponentImageConfig(repoRootDir, configKey, newValue string) error { +// Modify variables in the specified component image config file. +func modifyComponentImageConfig(repoRootDir, imageConfigRelPath, configKey, newValue string) error { // Check the configKey is valid desc, ok := componentImageConfigMap[configKey] if !ok { return fmt.Errorf("invalid component image config key: %s", configKey) } - logrus.WithField("repoDir", repoRootDir).WithField(configKey, newValue).Infof("Updating %s in %s", desc, componentImageConfigRelPath) + logrus.WithField("repoDir", repoRootDir).WithField(configKey, newValue).Infof("Updating %s in %s", desc, imageConfigRelPath) - if out, err := runCommandInDir(repoRootDir, "sed", []string{"-i", fmt.Sprintf(`s|%[1]s.*=.*".*"|%[1]s = "%[2]s"|`, regexp.QuoteMeta(configKey), regexp.QuoteMeta(newValue)), componentImageConfigRelPath}, nil); err != nil { + if out, err := runCommandInDir(repoRootDir, "sed", []string{"-i", fmt.Sprintf(`s|%[1]s.*=.*".*"|%[1]s = "%[2]s"|`, regexp.QuoteMeta(configKey), regexp.QuoteMeta(newValue)), imageConfigRelPath}, nil); err != nil { logrus.Error(out) - return fmt.Errorf("failed to update %s in %s: %w", desc, componentImageConfigRelPath, err) + return fmt.Errorf("failed to update %s in %s: %w", desc, imageConfigRelPath, err) } return nil } diff --git a/hack/release/checks.go b/hack/release/checks.go index 264a330735..fbb5bd8dd1 100644 --- a/hack/release/checks.go +++ b/hack/release/checks.go @@ -65,7 +65,7 @@ var checkVersionFormat = func(ctx context.Context, c *cli.Command) (context.Cont return ctx, nil } checkLog.Debug("Checking version format") - if isRelease, err := isReleaseVersionFormat(version); err != nil { + if isRelease, err := isValidReleaseVersion(version); err != nil { return ctx, fmt.Errorf("checking version format: %w", err) } else if !isRelease { return ctx, fmt.Errorf("provided version %q is not a valid release version. \n"+ diff --git a/hack/release/flags.go b/hack/release/flags.go index 86cf160e3c..148ca3081e 100644 --- a/hack/release/flags.go +++ b/hack/release/flags.go @@ -33,6 +33,13 @@ var debugFlag = &cli.BoolFlag{ Sources: cli.EnvVars("DEBUG"), } +var hookTimeoutFlag = &cli.DurationFlag{ + Name: "hook-timeout", + Usage: "Timeout duration for hook execution", + Sources: cli.EnvVars("HOOK_TIMEOUT"), + Value: DefaultHookTimeout, +} + // Git/GitHub related flags. var ( githubFlagCategory = "GitHub Options" @@ -121,7 +128,7 @@ var ( // No need to validate version for hashrelease return nil } - if valid, err := isReleaseVersionFormat(s); err != nil { + if valid, err := isValidReleaseVersion(s); err != nil { return fmt.Errorf("error validating version format: %w", err) } else if !valid { return fmt.Errorf("version %q is not a valid release version", s) @@ -314,6 +321,19 @@ var ( Sources: cli.EnvVars("CALICO_DIR"), Action: dirFlagCheck, } + calicoGitRepoFlag = &cli.StringFlag{ + Name: "calico-repo", + Category: calicoFlagCategory, + Usage: "The git repository to clone Calico from. Used when no Calico dir for CRDs is provided (development and testing purposes only)", + Sources: cli.EnvVars("CALICO_REPO"), + Value: "projectcalico/calico", + } + calicoGitBranchFlag = &cli.StringFlag{ + Name: "calico-branch", + Category: calicoFlagCategory, + Usage: "The git branch to clone Calico from. Used when no Calico dir for CRDs is provided (development and testing purposes only)", + Sources: cli.EnvVars("CALICO_BRANCH"), + } ) // Enterprise related flags. @@ -377,6 +397,19 @@ var ( Sources: cli.EnvVars("ENTERPRISE_DIR"), Action: dirFlagCheck, } + enterpriseGitRepoFlag = &cli.StringFlag{ + Name: "enterprise-repo", + Category: enterpriseFlagCategory, + Usage: "The git repository to clone Enterprise from. Used when no Enterprise dir for CRDs is provided (development and testing purposes only)", + Sources: cli.EnvVars("ENTERPRISE_REPO"), + Value: "tigera/calico-private", + } + enterpriseGitBranchFlag = &cli.StringFlag{ + Name: "enterprise-branch", + Category: enterpriseFlagCategory, + Usage: "The git branch to clone Enterprise from. Use in place of specificing the Enterprise dir for CRDs (development and testing purposes only)", + Sources: cli.EnvVars("ENTERPRISE_BRANCH", "CALICO_ENTERPRISE_BRANCH"), + } exceptEnterpriseFlag = &cli.StringSliceFlag{ Name: "except-calico-enterprise", Category: enterpriseFlagCategory, diff --git a/hack/release/hooks.go b/hack/release/hooks.go new file mode 100644 index 0000000000..e309da584c --- /dev/null +++ b/hack/release/hooks.go @@ -0,0 +1,233 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "context" + "errors" + "fmt" + "sync" + "time" + + "github.com/sirupsen/logrus" + "github.com/urfave/cli/v3" +) + +// HOOK SYSTEM DOCUMENTATION +// +// This package implements an extensible hook system that allows external code to extend +// the release build and publish workflows without modifying core logic. +// +// HOOK REGISTRATION: +// +// Hooks are registered via init() functions in separate files, Example: +// +// // myhooks.go +// func init() { +// buildPreHook = myBuildPreHook +// setupHashreleasePreHook = mySetupHashreleasePreHook +// publishPreHook = myPublishPreHook +// postPublishHook = myPostPublishHook +// } +// +// HOOK EXECUTION AND ERROR HANDLING: +// +// - Hooks are executed with a derived context that includes the main operation's context +// - If a hook times out, the error message clearly indicates which hook timed out +// - If a hook returns an error, it is wrapped with context about which hook failed +// - If a hook panics, the panic is NOT caught - letting it propagate to the caller +// - Hook errors are fatal and stop the entire operation +// - Timeout values can be configured via the --hook-timeout flag, +// and should be set based on expected hook execution time and overall operation time budget +// +// TIMEOUT CONFIGURATION: +// +// Timeout is configured via the --hook-timeout flag (default: 5 minutes). + +// DefaultHookTimeout is the default timeout for all hooks (5 minutes). +const DefaultHookTimeout = 5 * time.Minute + +// cliHookFunc is the signature for general purpose hooks that run during command execution. +type cliHookFunc func(ctx context.Context, c *cli.Command) error + +// cliBeforeHookFunc is the signature for hooks that run in the cli.BeforeFunc phase and can modify the context for subsequent actions. +type cliBeforeHookFunc func(ctx context.Context, c *cli.Command) (context.Context, error) + +// cliHookWithRepoDirFunc is the signature for hooks that require access to the repository root directory (e.g., hashrelease setup). +type cliHookWithRepoDirFunc func(ctx context.Context, c *cli.Command, repoRootDir string) (context.Context, error) + +// imageReleaseHookFunc is the signature for hooks that run in publish action after images are published (or skipped). +// It receives information about whether the release was newly published, allowing it to perform actions accordingly. +type imageReleaseHookFunc func(ctx context.Context, c *cli.Command, published bool) (context.Context, error) + +type cliHook struct { + Desc string + Hook cliHookFunc +} + +type multiHook struct { + mu sync.Mutex + hooks []cliHook +} + +func (h *multiHook) Add(desc string, hook cliHookFunc) { + h.mu.Lock() + defer h.mu.Unlock() + h.hooks = append(h.hooks, cliHook{ + Desc: desc, + Hook: hook, + }) +} + +func (h *multiHook) Hooks() []cliHook { + h.mu.Lock() + defer h.mu.Unlock() + return h.hooks +} + +func (h *multiHook) Reset() { + h.mu.Lock() + defer h.mu.Unlock() + h.hooks = nil +} + +type withTimeoutResult[T any] struct { + value T + err error +} + +// withTimeout is a generic helper that executes a function with timeout and error handling. +// It distinguishes between timeout and other errors and provides clear error messages. +// If the timeout is <= 0, it will use the DefaultHookTimeout. +// +// The hook function receives a cancellable context derived from the parent. On timeout or +// parent cancellation, this context is cancelled to signal the hook to stop. On success, +// the context is intentionally not cancelled so that any context the hook returns (which +// may be derived from it) remains valid for the caller. +// +// Hook functions should respect context cancellation (e.g., use context-aware I/O) +// to ensure the goroutine exits promptly when a timeout or cancellation occurs. +func withTimeout[T any](ctx context.Context, timeout time.Duration, hookName string, fn func(context.Context) (T, error)) (T, error) { + zero := new(T) // zero value for type T + if fn == nil { + return *zero, nil + } + if timeout <= 0 { + timeout = DefaultHookTimeout + } + logrus.WithFields(logrus.Fields{ + "hook": hookName, + "timeout": timeout, + }).Debug("Running hook") + + hookCtx, hookCancel := context.WithCancel(ctx) + // Cancel the hook context on any non-success path to signal the hook goroutine to stop. + // On success, hookCancel is NOT called because the returned value may be a context + // derived from hookCtx, and cancelling it would invalidate the caller's context. + succeeded := false + defer func() { + if !succeeded { + hookCancel() + } + }() + + timer := time.NewTimer(timeout) + done := make(chan withTimeoutResult[T], 1) + go func() { + value, err := fn(hookCtx) + done <- withTimeoutResult[T]{value, err} + }() + + select { + case r := <-done: + timer.Stop() + if r.err != nil { + return r.value, fmt.Errorf("%s failed: %w", hookName, r.err) + } + succeeded = true + return r.value, nil + case <-timer.C: + return *zero, fmt.Errorf("%s timed out after %v", hookName, timeout) + case <-ctx.Done(): + timer.Stop() + return *zero, fmt.Errorf("%s: parent context cancelled: %w", hookName, ctx.Err()) + } +} + +// RunBuildBeforeHook executes the buildBeforeHook with timeout and error handling. +func RunBuildBeforeHook(ctx context.Context, c *cli.Command, timeout time.Duration) (context.Context, error) { + if buildBeforeHook == nil { + return ctx, nil + } + return withTimeout(ctx, timeout, "buildBeforeHook", func(hookCtx context.Context) (context.Context, error) { + return buildBeforeHook(hookCtx, c) + }) +} + +// RunBuildAfterHook executes the buildAfterHook with timeout and error handling. +func RunBuildAfterHook(ctx context.Context, c *cli.Command, timeout time.Duration) error { + hooks := buildAfterHooks.Hooks() + if len(hooks) == 0 { + return nil + } + _, err := withTimeout(ctx, timeout, "buildAfterHook", func(hookCtx context.Context) (context.Context, error) { + // Run all registered build after hooks in LIFO order. + // All hooks run even if earlier ones fail, to ensure cleanup always happens. + var errs []error + for i := len(hooks) - 1; i >= 0; i-- { + h := hooks[i] + logrus.WithField("hook", h.Desc).Debug("Running build after hook") + if err := h.Hook(hookCtx, c); err != nil { + logrus.WithError(err).WithField("hook", h.Desc).Error("Build after hook failed") + errs = append(errs, fmt.Errorf("build after hook %q failed: %w", h.Desc, err)) + } + } + return hookCtx, errors.Join(errs...) + }) + return err +} + +// RunSetupHashreleasePreHook executes the setupHashreleasePreHook with timeout and error handling. +func RunSetupHashreleasePreHook(ctx context.Context, c *cli.Command, repoRootDir string, timeout time.Duration) (context.Context, error) { + if setupHashreleasePreHook == nil { + return ctx, nil + } + logrus.WithField("repoDir", repoRootDir).Debug("Running setupHashreleasePreHook") + return withTimeout(ctx, timeout, "setupHashreleasePreHook", func(hookCtx context.Context) (context.Context, error) { + return setupHashreleasePreHook(hookCtx, c, repoRootDir) + }) +} + +// RunPublishBeforeHook executes the publishBeforeHook with timeout and error handling. +// It wraps any errors to clearly indicate which hook failed and why. +func RunPublishBeforeHook(ctx context.Context, c *cli.Command, timeout time.Duration) (context.Context, error) { + if publishBeforeHook == nil { + return ctx, nil + } + return withTimeout(ctx, timeout, "publishBeforeHook", func(hookCtx context.Context) (context.Context, error) { + return publishBeforeHook(hookCtx, c) + }) +} + +// RunPublishImagePostHook executes the postPublishHook with timeout and error handling. +func RunPublishImagePostHook(ctx context.Context, c *cli.Command, published bool, timeout time.Duration) (context.Context, error) { + if publishImagePostHook == nil { + return ctx, nil + } + logrus.WithField("newlyPublished", published).Debug("Running postPublishHook") + return withTimeout(ctx, timeout, "postPublishHook", func(hookCtx context.Context) (context.Context, error) { + return publishImagePostHook(hookCtx, c, published) + }) +} diff --git a/hack/release/hooks_test.go b/hack/release/hooks_test.go new file mode 100644 index 0000000000..10ea07761e --- /dev/null +++ b/hack/release/hooks_test.go @@ -0,0 +1,293 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "context" + "errors" + "strings" + "testing" + "testing/synctest" + "time" + + "github.com/urfave/cli/v3" +) + +func TestWithTimeout(t *testing.T) { + t.Parallel() + + t.Run("nil function returns zero value", func(t *testing.T) { + t.Parallel() + val, err := withTimeout[string](context.Background(), time.Second, "test-hook", nil) + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + if val != "" { + t.Fatalf("expected empty string, got: %q", val) + } + }) + + t.Run("successful execution", func(t *testing.T) { + t.Parallel() + val, err := withTimeout(context.Background(), time.Second, "test-hook", func(ctx context.Context) (string, error) { + return "result", nil + }) + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + if val != "result" { + t.Fatalf("expected %q, got: %q", "result", val) + } + }) + + t.Run("function returns error", func(t *testing.T) { + t.Parallel() + _, err := withTimeout(context.Background(), time.Second, "test-hook", func(ctx context.Context) (string, error) { + return "", errors.New("hook error") + }) + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "test-hook failed") { + t.Fatalf("expected error to contain 'test-hook failed', got: %v", err) + } + if !strings.Contains(err.Error(), "hook error") { + t.Fatalf("expected error to contain 'hook error', got: %v", err) + } + }) + + t.Run("timeout triggers and cancels hook context", func(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { + _, err := withTimeout(context.Background(), 5*time.Minute, "slow-hook", func(ctx context.Context) (string, error) { + <-ctx.Done() + return "", ctx.Err() + }) + if err == nil { + t.Fatal("expected timeout error, got nil") + } + if !strings.Contains(err.Error(), "slow-hook timed out") { + t.Fatalf("expected timeout error message, got: %v", err) + } + }) + }) + + t.Run("parent context cancelled cancels hook context", func(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + go func() { + time.Sleep(1 * time.Minute) + cancel() + }() + _, err := withTimeout(ctx, 5*time.Minute, "cancel-hook", func(ctx context.Context) (string, error) { + // Hook respects context cancellation, so it exits when the parent is cancelled. + <-ctx.Done() + return "", ctx.Err() + }) + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "parent context cancelled") { + t.Fatalf("expected parent context cancelled error, got: %v", err) + } + }) + }) + + t.Run("zero timeout uses default", func(t *testing.T) { + t.Parallel() + val, err := withTimeout(context.Background(), 0, "test-hook", func(ctx context.Context) (int, error) { + return 42, nil + }) + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + if val != 42 { + t.Fatalf("expected 42, got: %d", val) + } + }) +} + +// TestRunBuildAfterHookRunsAll verifies that all build-after hooks run even if earlier ones fail. +func TestRunBuildAfterHookRunsAll(t *testing.T) { + defer buildAfterHooks.Reset() + + var ran []string + buildAfterHooks.Add("hook-1", func(ctx context.Context, c *cli.Command) error { + ran = append(ran, "hook-1") + return errors.New("hook-1 error") + }) + buildAfterHooks.Add("hook-2", func(ctx context.Context, c *cli.Command) error { + ran = append(ran, "hook-2") + return nil + }) + buildAfterHooks.Add("hook-3", func(ctx context.Context, c *cli.Command) error { + ran = append(ran, "hook-3") + return errors.New("hook-3 error") + }) + + err := RunBuildAfterHook(context.Background(), nil, 5*time.Second) + // Should have errors from hook-1 and hook-3 + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "hook-1 error") { + t.Fatalf("expected hook-1 error in result, got: %v", err) + } + if !strings.Contains(err.Error(), "hook-3 error") { + t.Fatalf("expected hook-3 error in result, got: %v", err) + } + // All three hooks should have run (in LIFO order: hook-3, hook-2, hook-1) + if len(ran) != 3 { + t.Fatalf("expected all 3 hooks to run, got %d: %v", len(ran), ran) + } + if ran[0] != "hook-3" || ran[1] != "hook-2" || ran[2] != "hook-1" { + t.Fatalf("expected LIFO order [hook-3, hook-2, hook-1], got %v", ran) + } +} + +// Run* hook tests must NOT be parallel since they mutate package-level hook vars. +func TestRunHookErrorPropagation(t *testing.T) { + cases := []struct { + name string + setup func(errMsg string) + run func() error + cleanup func() + }{ + { + name: "RunBuildBeforeHook", + setup: func(errMsg string) { + buildBeforeHook = func(ctx context.Context, c *cli.Command) (context.Context, error) { + return ctx, errors.New(errMsg) + } + }, + run: func() error { + _, err := RunBuildBeforeHook(context.Background(), nil, time.Second) + return err + }, + cleanup: func() { buildBeforeHook = nil }, + }, + { + name: "RunBuildAfterHook", + setup: func(errMsg string) { + buildAfterHooks.Add("test-hook", func(ctx context.Context, c *cli.Command) error { + return errors.New(errMsg) + }, + ) + }, + run: func() error { + return RunBuildAfterHook(context.Background(), nil, time.Second) + }, + cleanup: func() { buildAfterHooks.Reset() }, + }, + { + name: "RunSetupHashreleasePreHook", + setup: func(errMsg string) { + setupHashreleasePreHook = func(ctx context.Context, c *cli.Command, dir string) (context.Context, error) { + return ctx, errors.New(errMsg) + } + }, + run: func() error { + _, err := RunSetupHashreleasePreHook(context.Background(), nil, "/tmp", time.Second) + return err + }, + cleanup: func() { setupHashreleasePreHook = nil }, + }, + { + name: "RunPublishBeforeHook", + setup: func(errMsg string) { + publishBeforeHook = func(ctx context.Context, c *cli.Command) (context.Context, error) { + return ctx, errors.New(errMsg) + } + }, + run: func() error { + _, err := RunPublishBeforeHook(context.Background(), nil, time.Second) + return err + }, + cleanup: func() { publishBeforeHook = nil }, + }, + { + name: "RunPublishImagePostHook", + setup: func(errMsg string) { + publishImagePostHook = func(ctx context.Context, c *cli.Command, published bool) (context.Context, error) { + return ctx, errors.New(errMsg) + } + }, + run: func() error { + _, err := RunPublishImagePostHook(context.Background(), nil, true, time.Second) + return err + }, + cleanup: func() { publishImagePostHook = nil }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + defer tc.cleanup() + errMsg := tc.name + " failed" + tc.setup(errMsg) + err := tc.run() + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), errMsg) { + t.Fatalf("expected error to contain %q, got: %v", errMsg, err) + } + }) + } +} + +// Run* hook tests must NOT be parallel since it uses package-level hook vars that can be mutated by other tests. +func TestRunHooksNilNoOp(t *testing.T) { + ctx := context.Background() + + t.Run("RunBuildBeforeHook", func(t *testing.T) { + if rCtx, err := RunBuildBeforeHook(ctx, nil, time.Second); err != nil { + t.Fatalf("RunBuildPreHook with nil hook: %v", err) + } else if rCtx != ctx { + t.Fatal("RunBuildPreHook should return the original context when hook is nil") + } + }) + + t.Run("RunBuildAfterHook", func(t *testing.T) { + if err := RunBuildAfterHook(ctx, nil, time.Second); err != nil { + t.Fatalf("RunBuildAfterHook with nil hook: %v", err) + } + }) + + t.Run("RunSetupHashreleasePreHook", func(t *testing.T) { + if rCtx, err := RunSetupHashreleasePreHook(ctx, nil, "/tmp", time.Second); err != nil { + t.Fatalf("RunSetupHashreleasePreHook with nil hook: %v", err) + } else if rCtx != ctx { + t.Fatal("RunSetupHashreleasePreHook should return the original context when hook is nil") + } + }) + + t.Run("RunPublishBeforeHook", func(t *testing.T) { + if rCtx, err := RunPublishBeforeHook(ctx, nil, time.Second); err != nil { + t.Fatalf("RunPublishPreHook with nil hook: %v", err) + } else if rCtx != ctx { + t.Fatal("RunPublishPreHook should return the original context when hook is nil") + } + }) + + t.Run("RunPublishImagePostHook", func(t *testing.T) { + if rCtx, err := RunPublishImagePostHook(ctx, nil, true, time.Second); err != nil { + t.Fatalf("RunPostPublishHook with nil hook: %v", err) + } else if rCtx != ctx { + t.Fatal("RunPostPublishHook should return the original context when hook is nil") + } + }) +} diff --git a/hack/release/prep.go b/hack/release/prep.go index b38b589983..0e28b154fb 100644 --- a/hack/release/prep.go +++ b/hack/release/prep.go @@ -172,7 +172,7 @@ var prepAction = cli.ActionFunc(func(ctx context.Context, c *cli.Command) error // Update registry for Enterprise if eRegistry := c.String(enterpriseRegistryFlag.Name); eRegistry != "" { logrus.Debugf("Updating Enterprise registry to %s", eRegistry) - if err := modifyComponentImageConfig(repoRootDir, enterpriseRegistryConfigKey, eRegistry); err != nil { + if err := modifyComponentImageConfig(repoRootDir, componentImageConfigRelPath, enterpriseRegistryConfigKey, eRegistry); err != nil { return err } } diff --git a/hack/release/publish.go b/hack/release/publish.go index f36a4654e2..eba3c18043 100644 --- a/hack/release/publish.go +++ b/hack/release/publish.go @@ -43,23 +43,41 @@ var publishCommand = &cli.Command{ createGithubReleaseFlag, githubTokenFlag, draftGithubReleaseFlag, + hookTimeoutFlag, }, Before: publishBefore, Action: publishAction, } +// publishBeforeHook is an optional hook called at the start of publishBefore for additional pre-processing. +// It can be set via init() in separate files to extend the publish command behavior. +var publishBeforeHook cliBeforeHookFunc + +// publishImagePostHook is an optional hook called after images are published (or skipped). +// It receives whether a new release was published. It can be set via init() in separate files. +var publishImagePostHook imageReleaseHookFunc + // Pre-action for publish command. // It configures logging and performs validations. var publishBefore = cli.BeforeFunc(func(ctx context.Context, c *cli.Command) (context.Context, error) { configureLogging(c) + // Extract hook timeout once for reuse + hookTimeout := c.Duration(hookTimeoutFlag.Name) + + // Call pre-hook if registered. var err error + ctx, err = RunPublishBeforeHook(ctx, c, hookTimeout) + if err != nil { + return ctx, err + } + ctx, err = addRepoInfoToCtx(ctx, c.String(gitRepoFlag.Name)) if err != nil { return ctx, err } - // Run verison validations. This is a mandatory check. + // Run version validations. This is a mandatory check. ctx, err = checkVersion(ctx, c) if err != nil { return ctx, err @@ -99,10 +117,19 @@ var publishAction = cli.ActionFunc(func(ctx context.Context, c *cli.Command) err } // Publish images - if err := publishImages(c, repoRootDir); err != nil { + isNewRelease, err := publishImages(c, repoRootDir) + if err != nil { return err } + // Call post-publish hook if registered. + hookTimeout := c.Duration(hookTimeoutFlag.Name) + ctx, err = RunPublishImagePostHook(ctx, c, isNewRelease, hookTimeout) + if err != nil { + // Post-publish hook errors are non-fatal - images are already published + logrus.WithError(err).Warn("Post-publish hook failed (continuing as images are published)") + } + // Only images are published for hashrelease builds. if c.Bool(hashreleaseFlag.Name) { return nil @@ -118,15 +145,16 @@ var publishAction = cli.ActionFunc(func(ctx context.Context, c *cli.Command) err // Publish the operator images to the specified registry. // If the images are already published, it skips publishing. -func publishImages(c *cli.Command, repoRootDir string) error { +// Returns true if images were newly published, false if they already existed. +func publishImages(c *cli.Command, repoRootDir string) (bool, error) { version := c.String(versionFlag.Name) log := logrus.WithField("version", version) // Check if images are already published if published, err := operatorImagePublished(c); err != nil { - return fmt.Errorf("checking if images are already published: %w", err) + return false, fmt.Errorf("checking if images are already published: %w", err) } else if published { log.Warn("Images are already published") - return nil + return false, nil } // Set up environment variables for publish @@ -159,10 +187,10 @@ func publishImages(c *cli.Command, repoRootDir string) error { log.Info("Publishing Operator images") if out, err := makeInDir(repoRootDir, "release-publish-images", publishEnv...); err != nil { log.Error(out) - return fmt.Errorf("publishing images: %w", err) + return false, fmt.Errorf("publishing images: %w", err) } log.Info("Successfully published Operator images") - return nil + return true, nil } // Check if the operator image is already published. diff --git a/hack/release/utils.go b/hack/release/utils.go index 8a8c83c2fb..477246e99f 100644 --- a/hack/release/utils.go +++ b/hack/release/utils.go @@ -25,6 +25,7 @@ import ( "path/filepath" "regexp" "strings" + "syscall" "github.com/Masterminds/semver/v3" "github.com/sirupsen/logrus" @@ -132,6 +133,45 @@ func runCommandInDir(dir, name string, args, env []string) (string, error) { return strings.TrimSpace(outb.String()), err } +// runCommandInDirContext is a context-aware variant of runCommandInDir. +// When ctx is cancelled, the entire process group is killed to ensure child processes are cleaned up. +func runCommandInDirContext(ctx context.Context, dir, name string, args, env []string) (string, error) { + cmd := exec.CommandContext(ctx, name, args...) + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + cmd.Cancel = func() error { + return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + } + if len(env) != 0 { + cmd.Env = env + } + cmd.Dir = dir + var outb, errb bytes.Buffer + if logrus.IsLevelEnabled(logrus.DebugLevel) { + cmd.Stdout = io.MultiWriter(os.Stdout, &outb) + cmd.Stderr = io.MultiWriter(os.Stderr, &errb) + } else { + cmd.Stdout = io.MultiWriter(&outb) + cmd.Stderr = io.MultiWriter(&errb) + } + logrus.WithFields(logrus.Fields{ + "cmd": cmd.String(), + "dir": dir, + }).Debugf("Running %s command (context-aware)", name) + err := cmd.Run() + if err != nil { + errDesc := fmt.Sprintf(`running command "%s %s"`, name, strings.Join(args, " ")) + if dir != "" { + errDesc += fmt.Sprintf(" in directory %s", dir) + } + err = fmt.Errorf("%s: %w \n%s", errDesc, err, strings.TrimSpace(errb.String())) + } + return strings.TrimSpace(outb.String()), err +} + +func gitInDirContext(ctx context.Context, dir string, args ...string) (string, error) { + return runCommandInDirContext(ctx, dir, "git", args, nil) +} + func addRepoInfoToCtx(ctx context.Context, repo string) (context.Context, error) { if ctx.Value(githubOrgCtxKey) != nil && ctx.Value(githubRepoCtxKey) != nil { return ctx, nil @@ -198,6 +238,10 @@ func calicoVersions(repo, rootDir, operatorVersion string, local bool) (map[stri return versions, nil } +// isValidReleaseVersion validates the operator release version format. +// It defaults to standard release format (vX.Y.Z) but can be overridden if a different format is needed. +var isValidReleaseVersion = isReleaseVersionFormat + // isReleaseVersionFormat checks if the version in the format vX.Y.Z. func isReleaseVersionFormat(version string) (bool, error) { releaseRegex, err := regexp.Compile(releaseFormat) diff --git a/hack/release/utils_test.go b/hack/release/utils_test.go index 0fd666db27..991cac37fc 100644 --- a/hack/release/utils_test.go +++ b/hack/release/utils_test.go @@ -15,6 +15,7 @@ package main import ( + "context" "fmt" "os" "path/filepath" @@ -282,6 +283,60 @@ func TestAddTrailingSlash(t *testing.T) { } } +func TestIsValidReleaseVersionOverride(t *testing.T) { + orig := isValidReleaseVersion + defer func() { isValidReleaseVersion = orig }() + + t.Run("set to isReleaseVersionFormat", func(t *testing.T) { + isValidReleaseVersion = isReleaseVersionFormat + ok, err := isValidReleaseVersion("v1.2.3") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !ok { + t.Fatal("expected v1.2.3 to be valid with default validator") + } + }) + + t.Run("can be overridden", func(t *testing.T) { + // Override to accept enterprise format (vX.Y.Z-A.B) + isValidReleaseVersion = isEnterpriseReleaseVersionFormat + + ok, err := isValidReleaseVersion("v1.2.3-1.0") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !ok { + t.Fatal("expected v1.2.3-1.0 to be valid with enterprise validator") + } + }) +} + +func TestRunCommandInDirContext(t *testing.T) { + t.Parallel() + + t.Run("successful execution", func(t *testing.T) { + t.Parallel() + out, err := runCommandInDirContext(context.Background(), "", "echo", []string{"hello"}, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if out != "hello" { + t.Fatalf("expected %q, got %q", "hello", out) + } + }) + + t.Run("context cancellation kills process", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel immediately + _, err := runCommandInDirContext(ctx, "", "sleep", []string{"60"}, nil) + if err == nil { + t.Fatal("expected error from cancelled context, got nil") + } + }) +} + func TestIsPrereleaseEnterpriseVersion(t *testing.T) { t.Parallel() From e470142df855aec0041867f9c496e7e3049ac137 Mon Sep 17 00:00:00 2001 From: tuti Date: Sun, 22 Feb 2026 05:16:10 -0800 Subject: [PATCH 2/8] feat(release): add cloning repo for generating CRDs --- hack/release/build.go | 153 +++++++++++++++++++++++++++++++++++-- hack/release/build_test.go | 83 ++++++++++++++++++++ 2 files changed, 230 insertions(+), 6 deletions(-) create mode 100644 hack/release/build_test.go diff --git a/hack/release/build.go b/hack/release/build.go index ea6d31f7da..f8c2c25be2 100644 --- a/hack/release/build.go +++ b/hack/release/build.go @@ -21,6 +21,7 @@ import ( "path" "regexp" "runtime" + "strconv" "strings" "github.com/sirupsen/logrus" @@ -78,17 +79,22 @@ var buildCommand = &cli.Command{ calicoImagePathFlag, calicoVersionsConfigFlag, calicoDirFlag, + calicoGitRepoFlag, + calicoGitBranchFlag, enterpriseVersionFlag, enterpriseRegistryFlag, enterpriseImagePathFlag, enterpriseVersionsConfigFlag, enterpriseDirFlag, + enterpriseGitRepoFlag, + enterpriseGitBranchFlag, hashreleaseFlag, skipValidationFlag, hookTimeoutFlag, }, Before: buildBefore, Action: buildAction, + After: buildAfter, } // buildBeforeHook is an optional hook called at the start of buildBefore for additional pre-processing. @@ -159,8 +165,10 @@ var buildBefore = cli.BeforeFunc(func(ctx context.Context, c *cli.Command) (cont } // For hashrelease builds, ensure at least one of Calico or Enterprise version or versions file is specified. - // If Calico/Enterprise version build is selected, ensure CRDs directory is specified - // as the version will likely not exist as a tag/branch in the corresponding Calico/Enterprise repos. + // If Calico/Enterprise version build is selected, setup the dir for CRDs either by: + // - using the provided dir for CRDs if specified, or + // - cloning the corresponding repo at the git hash for the specific version and using the CRDs from there. + // // If Calico/Enterprise is built using versions file, log a warning if CRDs directory is not specified. calicoBuildType, calicoBuildOk := ctx.Value(calicoBuildCtxKey).(buildType) enterpriseBuildType, enterpriseBuildOk := ctx.Value(enterpriseBuildCtxKey).(buildType) @@ -168,16 +176,34 @@ var buildBefore = cli.BeforeFunc(func(ctx context.Context, c *cli.Command) (cont return ctx, fmt.Errorf("for hashrelease builds, at least one of Calico or Enterprise version or versions file must be specified") } if calicoBuildOk { - if calicoBuildType == versionBuild && c.String(calicoDirFlag.Name) == "" { - return ctx, fmt.Errorf("directory to calico repo must be specified for hashreleases built from calico version using %s flag", calicoDirFlag.Name) + if calicoBuildType == versionBuild { + repo := hashreleaseRepo{ + Product: "calico", + DirFlag: calicoDirFlag, + RepoFlag: calicoGitRepoFlag, + BranchFlag: calicoGitBranchFlag, + VersionFlag: calicoVersionFlag, + } + if err := repo.Setup(c); err != nil { + return ctx, fmt.Errorf("setting up Calico repo for hashrelease: %w", err) + } } if c.String(calicoDirFlag.Name) == "" { logrus.Warn("Calico directory not specified for hashrelease build, getting CRDs from default location may not be appropriate") } } if enterpriseBuildOk { - if enterpriseBuildType == versionBuild && c.String(enterpriseDirFlag.Name) == "" { - return ctx, fmt.Errorf("directory to enterprise repo must be specified for hashreleases built from enterprise version using %s flag", enterpriseDirFlag.Name) + if enterpriseBuildType == versionBuild { + repo := hashreleaseRepo{ + Product: "enterprise", + DirFlag: enterpriseDirFlag, + RepoFlag: enterpriseGitRepoFlag, + BranchFlag: enterpriseGitBranchFlag, + VersionFlag: enterpriseVersionFlag, + } + if err := repo.Setup(c); err != nil { + return ctx, fmt.Errorf("setting up Enterprise repo for hashrelease: %w", err) + } } if c.String(enterpriseDirFlag.Name) == "" { logrus.Warn("Enterprise directory not specified for hashrelease build, getting CRDs from default location may not be appropriate") @@ -401,3 +427,118 @@ func modifyComponentImageConfig(repoRootDir, imageConfigRelPath, configKey, newV } return nil } + +// extractGitHashFromVersion extracts the git hash from a version string. +// The version format is not strict, so long as it ends with g<12-char-hash>. +func extractGitHashFromVersion(version string) (string, error) { + re, err := regexp.Compile(`g([a-f0-9]{12})(-dirty)?$`) + if err != nil { + return "", fmt.Errorf("compiling git hash regex: %w", err) + } + matches := re.FindStringSubmatch(version) + if len(matches) < 2 { + return "", fmt.Errorf("no git hash found in version %s", version) + } + return matches[1], nil +} + +type hashreleaseRepo struct { + Product string + RepoFlag *cli.StringFlag + BranchFlag *cli.StringFlag + VersionFlag *cli.StringFlag + DirFlag *cli.StringFlag + repo string + branch string + version string +} + +func (r *hashreleaseRepo) Setup(c *cli.Command) error { + if dir := c.String(r.DirFlag.Name); dir != "" { + logrus.WithField("dir", dir).Infof("%s directory provided, skipping clone", r.Product) + return nil + } + r.repo = c.String(r.RepoFlag.Name) + r.version = c.String(r.VersionFlag.Name) + r.branch = c.String(r.BranchFlag.Name) + if r.branch == "" { + return fmt.Errorf("%s git branch not provided. Either set the %s dir or provide a branch", r.Product, r.Product) + } + if r.version == "" { + return fmt.Errorf("%s version not provided. Either set the %s dir or provide a version", r.Product, r.Product) + } + dir, err := r.clone() + if err != nil { + return fmt.Errorf("cloning %s repo: %w", r.Product, err) + } + if err := c.Set(r.DirFlag.Name, dir); err != nil { + return fmt.Errorf("setting %s dir flag: %w", r.Product, err) + } + return nil +} + +// cloneHashreleaseRepo clones the repo at the git hash that corresponds to the hashrelease version. +// It uses a shallow clone with incremental deepening (max depth: 100) to fetch the specific commit without downloading the entire history. +func (r *hashreleaseRepo) clone() (string, error) { + gitHash, err := extractGitHashFromVersion(r.version) + if err != nil { + return "", fmt.Errorf("extracting git hash from version: %w", err) + } + if gitHash == "" { + return "", fmt.Errorf("no git hash found in version %s", r.version) + } + repoTmpDir, err := os.MkdirTemp("", "enterprise-*") + if err != nil { + return "", fmt.Errorf("creating temp directory for %s repo: %w", r.Product, err) + } + buildAfterHooks.Add(fmt.Sprintf("Cleaning up %s repo temp directory", r.Product), func(ctx context.Context, c *cli.Command) error { + if err := os.RemoveAll(repoTmpDir); err != nil { + return fmt.Errorf("removing temp directory %s for %s repo: %w", repoTmpDir, r.Product, err) + } + return nil + }, + ) + remote := "origin" + logrus.WithFields(logrus.Fields{ + "version": r.version, + "gitHash": gitHash, + "remote": remote, + "dir": repoTmpDir, + }).Infof("Cloning %s repo at git hash", r.Product) + + // Init dir as a git repo to allow sparse checkout, which avoids downloading unnecessary files and history. + if _, err := git("-C", repoTmpDir, "init"); err != nil { + return repoTmpDir, fmt.Errorf("initializing git repo in %s temp dir: %w", r.Product, err) + } + if _, err := git("-C", repoTmpDir, "remote", "add", remote, fmt.Sprintf("git@github.com:%s.git", r.repo)); err != nil { + return repoTmpDir, fmt.Errorf("adding remote origin in %s temp git dir: %w", r.Product, err) + } + + // Shallow clone and incrementally deepen to either the specific commit or max depth + depth, deepen, maxDepth := 10, 20, 100 + if _, err := git("-C", repoTmpDir, "fetch", "--depth", strconv.Itoa(depth), remote, r.branch); err != nil { + return repoTmpDir, fmt.Errorf("fetching branch %s from remote in %s temp git dir: %w", r.branch, r.Product, err) + } + for { + // check if the commit is present after the fetch + if _, err := git("-C", repoTmpDir, "cat-file", "-e", gitHash+"^{commit}"); err == nil { + break + } + if depth >= maxDepth { + return repoTmpDir, fmt.Errorf("git hash %s not found after fetching with depth %d", gitHash, depth) + } + depth += deepen + logrus.WithField("depth", depth).Infof("Deepening git fetch for %s repo", r.Product) + if _, err := git("-C", repoTmpDir, "fetch", "--deepen", strconv.Itoa(deepen), "origin", r.branch); err != nil { + return repoTmpDir, fmt.Errorf("deepening fetch for branch %s in %s repo: %w", r.branch, r.Product, err) + } + } + + // Checkout the specific commit + if _, err := git("-C", repoTmpDir, "checkout", gitHash); err != nil { + return repoTmpDir, fmt.Errorf("checking out git hash %s in %s repo: %w", gitHash, r.Product, err) + } + + logrus.WithField("dir", repoTmpDir).Debugf("Successfully cloned %s repo", r.Product) + return repoTmpDir, nil +} diff --git a/hack/release/build_test.go b/hack/release/build_test.go new file mode 100644 index 0000000000..bdcf874a39 --- /dev/null +++ b/hack/release/build_test.go @@ -0,0 +1,83 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import "testing" + +func TestExtractGitHashFromVersion(t *testing.T) { + t.Parallel() + + cases := []struct { + version string + want string + wantErr bool + }{ + { + version: "v3.22.1", + wantErr: true, + }, + { + version: "v3.22.0-1.0", + wantErr: true, + }, + { + version: "v3.22.0-gshorthash", + wantErr: true, + }, + { + version: "v3.22.0-glonghashthatisnotvalid", + wantErr: true, + }, + { + version: "v3.22.1-25-g997f6be93484-extra", + wantErr: true, + }, + { + version: "v3.22.1-25-g997f6be93484-dirty", + want: "997f6be93484", + }, + { + version: "v3.22.1-948-g997f6be93484", + want: "997f6be93484", + }, + { + version: "v3.22.1-calient-0.dev-948-g1234567890ab", + want: "1234567890ab", + }, + { + version: "v3.23.0-2.0-calient-0.dev-948-gabcdef123456", + want: "abcdef123456", + }, + } + + for _, tc := range cases { + t.Run(tc.version, func(t *testing.T) { + t.Parallel() + got, err := extractGitHashFromVersion(tc.version) + if tc.wantErr { + if err == nil { + t.Fatalf("expected error, got none") + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tc.want { + t.Fatalf("extractGitHashFromVersion(%q) = %q, want %q", tc.version, got, tc.want) + } + }) + } +} From fc9ed2d8308136f41a57b943302e6c4de964d273 Mon Sep 17 00:00:00 2001 From: tuti Date: Sun, 22 Feb 2026 11:52:27 -0800 Subject: [PATCH 3/8] fix(release): update creating github release flag & validation --- Makefile | 2 +- hack/release/flags.go | 2 +- hack/release/publish.go | 7 +------ 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/Makefile b/Makefile index 8ceac5adb3..9e919c4f58 100644 --- a/Makefile +++ b/Makefile @@ -559,7 +559,7 @@ endif ## Create a release for the specified RELEASE_TAG. release-tag: var-require-all-RELEASE_TAG-GITHUB_TOKEN $(MAKE) release VERSION=$(RELEASE_TAG) - REPO=$(REPO) CREATE_GITHUB_RELEASE=true $(MAKE) release-publish VERSION=$(RELEASE_TAG) + REPO=$(REPO) $(MAKE) release-publish VERSION=$(RELEASE_TAG) ## Generate release notes for the specified VERSION. release-notes: hack/bin/release var-require-all-VERSION-GITHUB_TOKEN diff --git a/hack/release/flags.go b/hack/release/flags.go index 148ca3081e..124c13c427 100644 --- a/hack/release/flags.go +++ b/hack/release/flags.go @@ -80,7 +80,7 @@ var ( Category: githubFlagCategory, Usage: "Create a GitHub release", Sources: cli.EnvVars("CREATE_GITHUB_RELEASE"), - Value: false, + Value: true, Action: func(ctx context.Context, c *cli.Command, b bool) error { if b && c.String(githubTokenFlag.Name) == "" { return fmt.Errorf("github-token is required to create GitHub releases") diff --git a/hack/release/publish.go b/hack/release/publish.go index eba3c18043..8a1ad1609f 100644 --- a/hack/release/publish.go +++ b/hack/release/publish.go @@ -89,12 +89,7 @@ var publishBefore = cli.BeforeFunc(func(ctx context.Context, c *cli.Command) (co return ctx, nil } - // If building a hashrelease, publishGithubRelease must be false - if c.Bool(hashreleaseFlag.Name) && c.Bool(createGithubReleaseFlag.Name) { - return ctx, fmt.Errorf("cannot publish GitHub release for hashrelease builds") - } - - if !c.Bool(createGithubReleaseFlag.Name) { + if c.Bool(hashreleaseFlag.Name) || !c.Bool(createGithubReleaseFlag.Name) { return ctx, nil } From ac32634ea244c278f74acd0996efc6d21912f544 Mon Sep 17 00:00:00 2001 From: tuti Date: Mon, 23 Feb 2026 15:24:20 -0800 Subject: [PATCH 4/8] fix: typos --- hack/release/build.go | 4 ++-- hack/release/flags.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/hack/release/build.go b/hack/release/build.go index f8c2c25be2..c981f8abff 100644 --- a/hack/release/build.go +++ b/hack/release/build.go @@ -106,7 +106,7 @@ var buildBeforeHook cliBeforeHookFunc var setupHashreleasePreHook cliHookWithRepoDirFunc // buildAfterHook is an optional hook called in buildAfter for additional post-build tasks. -// This run regardless of build success or failure, so it can be used for cleanup tasks. +// This runs regardless of build success or failure, so it can be used for cleanup tasks. var buildAfterHooks multiHook // Pre-action for release build command. @@ -487,7 +487,7 @@ func (r *hashreleaseRepo) clone() (string, error) { if gitHash == "" { return "", fmt.Errorf("no git hash found in version %s", r.version) } - repoTmpDir, err := os.MkdirTemp("", "enterprise-*") + repoTmpDir, err := os.MkdirTemp("", r.Product+"-*") if err != nil { return "", fmt.Errorf("creating temp directory for %s repo: %w", r.Product, err) } diff --git a/hack/release/flags.go b/hack/release/flags.go index 124c13c427..19bebfbaf3 100644 --- a/hack/release/flags.go +++ b/hack/release/flags.go @@ -407,7 +407,7 @@ var ( enterpriseGitBranchFlag = &cli.StringFlag{ Name: "enterprise-branch", Category: enterpriseFlagCategory, - Usage: "The git branch to clone Enterprise from. Use in place of specificing the Enterprise dir for CRDs (development and testing purposes only)", + Usage: "The git branch to clone Enterprise from. Use in place of specifying the Enterprise dir for CRDs (development and testing purposes only)", Sources: cli.EnvVars("ENTERPRISE_BRANCH", "CALICO_ENTERPRISE_BRANCH"), } exceptEnterpriseFlag = &cli.StringSliceFlag{ From 91fb235f895cb3ca403b09ebcd98c8519d051a71 Mon Sep 17 00:00:00 2001 From: tuti Date: Tue, 24 Feb 2026 17:15:11 -0800 Subject: [PATCH 5/8] updates based on review --- hack/release/build.go | 33 ++++++++++++++++++++++++--------- hack/release/build_test.go | 2 +- hack/release/hooks.go | 8 +++----- hack/release/hooks_test.go | 4 ++-- hack/release/utils.go | 3 +++ 5 files changed, 33 insertions(+), 17 deletions(-) diff --git a/hack/release/build.go b/hack/release/build.go index c981f8abff..e2ec91bfed 100644 --- a/hack/release/build.go +++ b/hack/release/build.go @@ -16,6 +16,7 @@ package main import ( "context" + "errors" "fmt" "os" "path" @@ -431,7 +432,10 @@ func modifyComponentImageConfig(repoRootDir, imageConfigRelPath, configKey, newV // extractGitHashFromVersion extracts the git hash from a version string. // The version format is not strict, so long as it ends with g<12-char-hash>. func extractGitHashFromVersion(version string) (string, error) { - re, err := regexp.Compile(`g([a-f0-9]{12})(-dirty)?$`) + if strings.HasSuffix(version, "-dirty") { + return "", fmt.Errorf("version %s indicates a dirty git state, cannot extract git hash", version) + } + re, err := regexp.Compile(`g([a-f0-9]{12})?$`) if err != nil { return "", fmt.Errorf("compiling git hash regex: %w", err) } @@ -461,11 +465,15 @@ func (r *hashreleaseRepo) Setup(c *cli.Command) error { r.repo = c.String(r.RepoFlag.Name) r.version = c.String(r.VersionFlag.Name) r.branch = c.String(r.BranchFlag.Name) + var errStack error if r.branch == "" { - return fmt.Errorf("%s git branch not provided. Either set the %s dir or provide a branch", r.Product, r.Product) + errStack = errors.Join(errStack, fmt.Errorf("%s git branch not provided. Either set the %s dir or provide a branch", r.Product, r.Product)) } if r.version == "" { - return fmt.Errorf("%s version not provided. Either set the %s dir or provide a version", r.Product, r.Product) + errStack = errors.Join(errStack, fmt.Errorf("%s version not provided. Either set the %s dir or provide a version", r.Product, r.Product)) + } + if errStack != nil { + return errStack } dir, err := r.clone() if err != nil { @@ -480,6 +488,13 @@ func (r *hashreleaseRepo) Setup(c *cli.Command) error { // cloneHashreleaseRepo clones the repo at the git hash that corresponds to the hashrelease version. // It uses a shallow clone with incremental deepening (max depth: 100) to fetch the specific commit without downloading the entire history. func (r *hashreleaseRepo) clone() (string, error) { + repoPattern, err := regexp.Compile(`^[\w-]+/[\w.-]+$`) + if err != nil { + return "", fmt.Errorf("compiling repo name regex: %w", err) + } + if !repoPattern.MatchString(r.repo) { + return "", fmt.Errorf("invalid repo format %s, expected format owner/repo", r.repo) + } gitHash, err := extractGitHashFromVersion(r.version) if err != nil { return "", fmt.Errorf("extracting git hash from version: %w", err) @@ -508,16 +523,16 @@ func (r *hashreleaseRepo) clone() (string, error) { // Init dir as a git repo to allow sparse checkout, which avoids downloading unnecessary files and history. if _, err := git("-C", repoTmpDir, "init"); err != nil { - return repoTmpDir, fmt.Errorf("initializing git repo in %s temp dir: %w", r.Product, err) + return "", fmt.Errorf("initializing git repo in %s temp dir: %w", r.Product, err) } if _, err := git("-C", repoTmpDir, "remote", "add", remote, fmt.Sprintf("git@github.com:%s.git", r.repo)); err != nil { - return repoTmpDir, fmt.Errorf("adding remote origin in %s temp git dir: %w", r.Product, err) + return "", fmt.Errorf("adding remote origin in %s temp git dir: %w", r.Product, err) } // Shallow clone and incrementally deepen to either the specific commit or max depth depth, deepen, maxDepth := 10, 20, 100 if _, err := git("-C", repoTmpDir, "fetch", "--depth", strconv.Itoa(depth), remote, r.branch); err != nil { - return repoTmpDir, fmt.Errorf("fetching branch %s from remote in %s temp git dir: %w", r.branch, r.Product, err) + return "", fmt.Errorf("fetching branch %s from remote in %s temp git dir: %w", r.branch, r.Product, err) } for { // check if the commit is present after the fetch @@ -525,18 +540,18 @@ func (r *hashreleaseRepo) clone() (string, error) { break } if depth >= maxDepth { - return repoTmpDir, fmt.Errorf("git hash %s not found after fetching with depth %d", gitHash, depth) + return "", fmt.Errorf("git hash %s not found after fetching with depth %d", gitHash, depth) } depth += deepen logrus.WithField("depth", depth).Infof("Deepening git fetch for %s repo", r.Product) if _, err := git("-C", repoTmpDir, "fetch", "--deepen", strconv.Itoa(deepen), "origin", r.branch); err != nil { - return repoTmpDir, fmt.Errorf("deepening fetch for branch %s in %s repo: %w", r.branch, r.Product, err) + return "", fmt.Errorf("deepening fetch for branch %s in %s repo: %w", r.branch, r.Product, err) } } // Checkout the specific commit if _, err := git("-C", repoTmpDir, "checkout", gitHash); err != nil { - return repoTmpDir, fmt.Errorf("checking out git hash %s in %s repo: %w", gitHash, r.Product, err) + return "", fmt.Errorf("checking out git hash %s in %s repo: %w", gitHash, r.Product, err) } logrus.WithField("dir", repoTmpDir).Debugf("Successfully cloned %s repo", r.Product) diff --git a/hack/release/build_test.go b/hack/release/build_test.go index bdcf874a39..d6b2efb51a 100644 --- a/hack/release/build_test.go +++ b/hack/release/build_test.go @@ -46,7 +46,7 @@ func TestExtractGitHashFromVersion(t *testing.T) { }, { version: "v3.22.1-25-g997f6be93484-dirty", - want: "997f6be93484", + wantErr: true, }, { version: "v3.22.1-948-g997f6be93484", diff --git a/hack/release/hooks.go b/hack/release/hooks.go index e309da584c..77a3652a42 100644 --- a/hack/release/hooks.go +++ b/hack/release/hooks.go @@ -18,6 +18,7 @@ import ( "context" "errors" "fmt" + "slices" "sync" "time" @@ -36,10 +37,7 @@ import ( // // // myhooks.go // func init() { -// buildPreHook = myBuildPreHook -// setupHashreleasePreHook = mySetupHashreleasePreHook -// publishPreHook = myPublishPreHook -// postPublishHook = myPostPublishHook +// buildBeforeHook = myBuildBeforeHook // } // // HOOK EXECUTION AND ERROR HANDLING: @@ -94,7 +92,7 @@ func (h *multiHook) Add(desc string, hook cliHookFunc) { func (h *multiHook) Hooks() []cliHook { h.mu.Lock() defer h.mu.Unlock() - return h.hooks + return slices.Clone(h.hooks) } func (h *multiHook) Reset() { diff --git a/hack/release/hooks_test.go b/hack/release/hooks_test.go index 10ea07761e..8656ef4d08 100644 --- a/hack/release/hooks_test.go +++ b/hack/release/hooks_test.go @@ -120,7 +120,7 @@ func TestWithTimeout(t *testing.T) { }) } -// TestRunBuildAfterHookRunsAll verifies that all build-after hooks run even if earlier ones fail. +// Run* hook tests must NOT be parallel since it uses package-level hook vars that can be mutated by other tests. func TestRunBuildAfterHookRunsAll(t *testing.T) { defer buildAfterHooks.Reset() @@ -158,7 +158,7 @@ func TestRunBuildAfterHookRunsAll(t *testing.T) { } } -// Run* hook tests must NOT be parallel since they mutate package-level hook vars. +// Run* hook tests must NOT be parallel since it uses package-level hook vars that can be mutated by other tests. func TestRunHookErrorPropagation(t *testing.T) { cases := []struct { name string diff --git a/hack/release/utils.go b/hack/release/utils.go index 477246e99f..9c234237b8 100644 --- a/hack/release/utils.go +++ b/hack/release/utils.go @@ -139,6 +139,9 @@ func runCommandInDirContext(ctx context.Context, dir, name string, args, env []s cmd := exec.CommandContext(ctx, name, args...) cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} cmd.Cancel = func() error { + if cmd.Process == nil { + return nil + } return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) } if len(env) != 0 { From 071db3cbe075ea091611d9c36e47a08408072d1c Mon Sep 17 00:00:00 2001 From: tuti Date: Thu, 26 Feb 2026 12:23:49 -0800 Subject: [PATCH 6/8] fix: address review comment - switch phase hook to multiHook - clean up cloning hashrelease repo to use treeless clone instead --- hack/release/build.go | 53 +++++--------- hack/release/hooks.go | 142 +++++++++++++++++++++++-------------- hack/release/hooks_test.go | 32 ++++----- hack/release/publish.go | 8 +-- 4 files changed, 125 insertions(+), 110 deletions(-) diff --git a/hack/release/build.go b/hack/release/build.go index e2ec91bfed..9c3eed7268 100644 --- a/hack/release/build.go +++ b/hack/release/build.go @@ -22,7 +22,6 @@ import ( "path" "regexp" "runtime" - "strconv" "strings" "github.com/sirupsen/logrus" @@ -100,15 +99,15 @@ var buildCommand = &cli.Command{ // buildBeforeHook is an optional hook called at the start of buildBefore for additional pre-processing. // It can be set via init() in separate files to extend the build command behavior. -var buildBeforeHook cliBeforeHookFunc +var buildBeforeHook multiHook[cliBeforeHookFunc] -// setupHashreleasePreHook is an optional hook called at the start of setupHashreleaseBuild +// setupHashreleaseBeforeHook is an optional hook called at the start of setupHashreleaseBuild // for additional hashrelease setup (e.g., copying image config files). -var setupHashreleasePreHook cliHookWithRepoDirFunc +var setupHashreleaseBeforeHook multiHook[cliHookWithRepoDirFunc] // buildAfterHook is an optional hook called in buildAfter for additional post-build tasks. // This runs regardless of build success or failure, so it can be used for cleanup tasks. -var buildAfterHooks multiHook +var buildAfterHooks multiHook[cliHookFunc] // Pre-action for release build command. var buildBefore = cli.BeforeFunc(func(ctx context.Context, c *cli.Command) (context.Context, error) { @@ -336,7 +335,7 @@ func setupHashreleaseBuild(ctx context.Context, c *cli.Command, repoRootDir stri // Call pre-hook if registered. var err error hookTimeout := c.Duration(hookTimeoutFlag.Name) - if ctx, err = RunSetupHashreleasePreHook(ctx, c, repoRootDir, hookTimeout); err != nil { + if ctx, err = RunSetupHashreleaseBeforeHook(ctx, c, repoRootDir, hookTimeout); err != nil { return fmt.Errorf("setup hashrelease pre-hook failed: %w", err) } @@ -486,8 +485,8 @@ func (r *hashreleaseRepo) Setup(c *cli.Command) error { } // cloneHashreleaseRepo clones the repo at the git hash that corresponds to the hashrelease version. -// It uses a shallow clone with incremental deepening (max depth: 100) to fetch the specific commit without downloading the entire history. func (r *hashreleaseRepo) clone() (string, error) { + // Validate repo format (owner/repo) repoPattern, err := regexp.Compile(`^[\w-]+/[\w.-]+$`) if err != nil { return "", fmt.Errorf("compiling repo name regex: %w", err) @@ -495,6 +494,8 @@ func (r *hashreleaseRepo) clone() (string, error) { if !repoPattern.MatchString(r.repo) { return "", fmt.Errorf("invalid repo format %s, expected format owner/repo", r.repo) } + + // Extract git hash from version to know which commit we need. gitHash, err := extractGitHashFromVersion(r.version) if err != nil { return "", fmt.Errorf("extracting git hash from version: %w", err) @@ -502,6 +503,8 @@ func (r *hashreleaseRepo) clone() (string, error) { if gitHash == "" { return "", fmt.Errorf("no git hash found in version %s", r.version) } + + // Create a temp directory for cloning the repo. We will clean this up in a build after hook. repoTmpDir, err := os.MkdirTemp("", r.Product+"-*") if err != nil { return "", fmt.Errorf("creating temp directory for %s repo: %w", r.Product, err) @@ -521,39 +524,15 @@ func (r *hashreleaseRepo) clone() (string, error) { "dir": repoTmpDir, }).Infof("Cloning %s repo at git hash", r.Product) - // Init dir as a git repo to allow sparse checkout, which avoids downloading unnecessary files and history. - if _, err := git("-C", repoTmpDir, "init"); err != nil { - return "", fmt.Errorf("initializing git repo in %s temp dir: %w", r.Product, err) - } - if _, err := git("-C", repoTmpDir, "remote", "add", remote, fmt.Sprintf("git@github.com:%s.git", r.repo)); err != nil { - return "", fmt.Errorf("adding remote origin in %s temp git dir: %w", r.Product, err) + // Create a treeless clone that gives access to the commit history without downloading all the blobs. + if _, err := git("clone", "--filter=tree:0", "--no-checkout", "-b", r.branch, fmt.Sprintf("git@github.com:%s.git", r.repo), repoTmpDir); err != nil { + return "", fmt.Errorf("cloning %s git repo intotemp dir: %w", r.Product, err) } - // Shallow clone and incrementally deepen to either the specific commit or max depth - depth, deepen, maxDepth := 10, 20, 100 - if _, err := git("-C", repoTmpDir, "fetch", "--depth", strconv.Itoa(depth), remote, r.branch); err != nil { - return "", fmt.Errorf("fetching branch %s from remote in %s temp git dir: %w", r.branch, r.Product, err) + // Detached checkout of the commit we want; this will automatically fetch whatever blobs we need + if _, err := gitInDir(repoTmpDir, "switch", "--detach", gitHash); err != nil { + return "", fmt.Errorf("switching %s repo to detached commit %s: %w", r.Product, gitHash, err) } - for { - // check if the commit is present after the fetch - if _, err := git("-C", repoTmpDir, "cat-file", "-e", gitHash+"^{commit}"); err == nil { - break - } - if depth >= maxDepth { - return "", fmt.Errorf("git hash %s not found after fetching with depth %d", gitHash, depth) - } - depth += deepen - logrus.WithField("depth", depth).Infof("Deepening git fetch for %s repo", r.Product) - if _, err := git("-C", repoTmpDir, "fetch", "--deepen", strconv.Itoa(deepen), "origin", r.branch); err != nil { - return "", fmt.Errorf("deepening fetch for branch %s in %s repo: %w", r.branch, r.Product, err) - } - } - - // Checkout the specific commit - if _, err := git("-C", repoTmpDir, "checkout", gitHash); err != nil { - return "", fmt.Errorf("checking out git hash %s in %s repo: %w", gitHash, r.Product, err) - } - logrus.WithField("dir", repoTmpDir).Debugf("Successfully cloned %s repo", r.Product) return repoTmpDir, nil } diff --git a/hack/release/hooks.go b/hack/release/hooks.go index 77a3652a42..509fdedbee 100644 --- a/hack/release/hooks.go +++ b/hack/release/hooks.go @@ -31,22 +31,34 @@ import ( // This package implements an extensible hook system that allows external code to extend // the release build and publish workflows without modifying core logic. // +// HOOK NAME AND SIGNATURE CONVENTIONS: +// - Workflow hooks are multiHook variables that can have multiple registered hooks. +// See "HOOKS EXECUTION AND ERROR HANDLING" below for execution order and error handling details. +// - Hooks are categorized by the phase of the workflow they run in and follow the naming convention: Hook. For example: +// - buildBeforeHook: runs before the build phase +// - publishImagesAfterHook: runs after the publish images phase +// - Each hook type has a specific function signature that defines the parameters it receives and if it can modify the context for subsequent actions. For example: +// - cliHookFunc does not modify the context: func(ctx context.Context, c *cli.Command) error +// - cliBeforeHookFunc can modify the context: func(ctx context.Context, c *cli.Command) (context.Context, error) +// // HOOK REGISTRATION: // -// Hooks are registered via init() functions in separate files, Example: +// Hooks are registered by calling the Add method on the appropriate multiHook variable, typically from an init() function. For example: // // // myhooks.go // func init() { -// buildBeforeHook = myBuildBeforeHook +// buildBeforeHook.Add("my custom before hook", myCustomBuildBeforeHook) +// buildAfterHook.Add("my custom after hook", myCustomBuildAfterHook) // } // // HOOK EXECUTION AND ERROR HANDLING: // // - Hooks are executed with a derived context that includes the main operation's context +// - Hooks that run in the "Before" phase (e.g., buildBeforeHook) run in registration order and stop on the first error (fail-fast). +// - Hooks that run in the "After" phase (e.g., buildAfterHook) run in reverse registration order and collect all errors, returning them as a single error. // - If a hook times out, the error message clearly indicates which hook timed out // - If a hook returns an error, it is wrapped with context about which hook failed // - If a hook panics, the panic is NOT caught - letting it propagate to the caller -// - Hook errors are fatal and stop the entire operation // - Timeout values can be configured via the --hook-timeout flag, // and should be set based on expected hook execution time and overall operation time budget // @@ -70,32 +82,32 @@ type cliHookWithRepoDirFunc func(ctx context.Context, c *cli.Command, repoRootDi // It receives information about whether the release was newly published, allowing it to perform actions accordingly. type imageReleaseHookFunc func(ctx context.Context, c *cli.Command, published bool) (context.Context, error) -type cliHook struct { +type cliHook[T any] struct { Desc string - Hook cliHookFunc + Hook T } -type multiHook struct { +type multiHook[T any] struct { mu sync.Mutex - hooks []cliHook + hooks []cliHook[T] } -func (h *multiHook) Add(desc string, hook cliHookFunc) { +func (h *multiHook[T]) Add(desc string, hook T) { h.mu.Lock() defer h.mu.Unlock() - h.hooks = append(h.hooks, cliHook{ + h.hooks = append(h.hooks, cliHook[T]{ Desc: desc, Hook: hook, }) } -func (h *multiHook) Hooks() []cliHook { +func (h *multiHook[T]) Hooks() []cliHook[T] { h.mu.Lock() defer h.mu.Unlock() return slices.Clone(h.hooks) } -func (h *multiHook) Reset() { +func (h *multiHook[T]) Reset() { h.mu.Lock() defer h.mu.Unlock() h.hooks = nil @@ -164,68 +176,92 @@ func withTimeout[T any](ctx context.Context, timeout time.Duration, hookName str } } -// RunBuildBeforeHook executes the buildBeforeHook with timeout and error handling. -func RunBuildBeforeHook(ctx context.Context, c *cli.Command, timeout time.Duration) (context.Context, error) { - if buildBeforeHook == nil { +// runHooksFailFast runs all hooks in registration order, stopping on the first error. +func runHooksFailFast[T any]( + ctx context.Context, + timeout time.Duration, + hookName string, + mh *multiHook[T], + run func(T, context.Context) (context.Context, error), +) (context.Context, error) { + hooks := mh.Hooks() + if len(hooks) == 0 { return ctx, nil } - return withTimeout(ctx, timeout, "buildBeforeHook", func(hookCtx context.Context) (context.Context, error) { - return buildBeforeHook(hookCtx, c) + return withTimeout(ctx, timeout, hookName, func(hookCtx context.Context) (context.Context, error) { + for _, h := range hooks { + var err error + hookCtx, err = run(h.Hook, hookCtx) + if err != nil { + return hookCtx, fmt.Errorf("%s %q failed: %w", hookName, h.Desc, err) + } + } + return hookCtx, nil }) } -// RunBuildAfterHook executes the buildAfterHook with timeout and error handling. -func RunBuildAfterHook(ctx context.Context, c *cli.Command, timeout time.Duration) error { - hooks := buildAfterHooks.Hooks() +// runHooksCollectErrors runs all hooks in reverse (LIFO) order, collecting all errors. +func runHooksCollectErrors[T any]( + ctx context.Context, + timeout time.Duration, + hookName string, + mh *multiHook[T], + run func(T, context.Context) (context.Context, error), +) (context.Context, error) { + hooks := mh.Hooks() if len(hooks) == 0 { - return nil + return ctx, nil } - _, err := withTimeout(ctx, timeout, "buildAfterHook", func(hookCtx context.Context) (context.Context, error) { - // Run all registered build after hooks in LIFO order. - // All hooks run even if earlier ones fail, to ensure cleanup always happens. + return withTimeout(ctx, timeout, hookName, func(hookCtx context.Context) (context.Context, error) { var errs []error for i := len(hooks) - 1; i >= 0; i-- { - h := hooks[i] - logrus.WithField("hook", h.Desc).Debug("Running build after hook") - if err := h.Hook(hookCtx, c); err != nil { - logrus.WithError(err).WithField("hook", h.Desc).Error("Build after hook failed") - errs = append(errs, fmt.Errorf("build after hook %q failed: %w", h.Desc, err)) + var err error + hookCtx, err = run(hooks[i].Hook, hookCtx) + if err != nil { + errs = append(errs, fmt.Errorf("%s %q failed: %w", hookName, hooks[i].Desc, err)) } } return hookCtx, errors.Join(errs...) }) +} + +// RunBuildBeforeHook executes the buildBeforeHook with timeout and error handling. +func RunBuildBeforeHook(ctx context.Context, c *cli.Command, timeout time.Duration) (context.Context, error) { + return runHooksFailFast(ctx, timeout, "buildBeforeHook", &buildBeforeHook, + func(h cliBeforeHookFunc, ctx context.Context) (context.Context, error) { + return h(ctx, c) + }) +} + +// RunBuildAfterHook executes the buildAfterHook with timeout and error handling. +func RunBuildAfterHook(ctx context.Context, c *cli.Command, timeout time.Duration) error { + _, err := runHooksCollectErrors(ctx, timeout, "buildAfterHook", &buildAfterHooks, + func(h cliHookFunc, ctx context.Context) (context.Context, error) { + return ctx, h(ctx, c) + }) return err } -// RunSetupHashreleasePreHook executes the setupHashreleasePreHook with timeout and error handling. -func RunSetupHashreleasePreHook(ctx context.Context, c *cli.Command, repoRootDir string, timeout time.Duration) (context.Context, error) { - if setupHashreleasePreHook == nil { - return ctx, nil - } - logrus.WithField("repoDir", repoRootDir).Debug("Running setupHashreleasePreHook") - return withTimeout(ctx, timeout, "setupHashreleasePreHook", func(hookCtx context.Context) (context.Context, error) { - return setupHashreleasePreHook(hookCtx, c, repoRootDir) - }) +// RunSetupHashreleaseBeforeHook executes the setupHashreleaseBeforeHook with timeout and error handling. +func RunSetupHashreleaseBeforeHook(ctx context.Context, c *cli.Command, repoRootDir string, timeout time.Duration) (context.Context, error) { + return runHooksFailFast(ctx, timeout, "setupHashreleaseBeforeHook", &setupHashreleaseBeforeHook, + func(h cliHookWithRepoDirFunc, ctx context.Context) (context.Context, error) { + return h(ctx, c, repoRootDir) + }) } // RunPublishBeforeHook executes the publishBeforeHook with timeout and error handling. -// It wraps any errors to clearly indicate which hook failed and why. func RunPublishBeforeHook(ctx context.Context, c *cli.Command, timeout time.Duration) (context.Context, error) { - if publishBeforeHook == nil { - return ctx, nil - } - return withTimeout(ctx, timeout, "publishBeforeHook", func(hookCtx context.Context) (context.Context, error) { - return publishBeforeHook(hookCtx, c) - }) + return runHooksFailFast(ctx, timeout, "publishBeforeHook", &publishBeforeHook, + func(h cliBeforeHookFunc, ctx context.Context) (context.Context, error) { + return h(ctx, c) + }) } -// RunPublishImagePostHook executes the postPublishHook with timeout and error handling. -func RunPublishImagePostHook(ctx context.Context, c *cli.Command, published bool, timeout time.Duration) (context.Context, error) { - if publishImagePostHook == nil { - return ctx, nil - } - logrus.WithField("newlyPublished", published).Debug("Running postPublishHook") - return withTimeout(ctx, timeout, "postPublishHook", func(hookCtx context.Context) (context.Context, error) { - return publishImagePostHook(hookCtx, c, published) - }) +// RunPublishImageAfterHook executes the publishImageAfterHook with timeout and error handling. +func RunPublishImageAfterHook(ctx context.Context, c *cli.Command, published bool, timeout time.Duration) (context.Context, error) { + return runHooksCollectErrors(ctx, timeout, "publishImageAfterHook", &publishImageAfterHook, + func(h imageReleaseHookFunc, ctx context.Context) (context.Context, error) { + return h(ctx, c, published) + }) } diff --git a/hack/release/hooks_test.go b/hack/release/hooks_test.go index 8656ef4d08..a2be270b52 100644 --- a/hack/release/hooks_test.go +++ b/hack/release/hooks_test.go @@ -169,15 +169,15 @@ func TestRunHookErrorPropagation(t *testing.T) { { name: "RunBuildBeforeHook", setup: func(errMsg string) { - buildBeforeHook = func(ctx context.Context, c *cli.Command) (context.Context, error) { + buildBeforeHook.Add("test-hook", func(ctx context.Context, c *cli.Command) (context.Context, error) { return ctx, errors.New(errMsg) - } + }) }, run: func() error { _, err := RunBuildBeforeHook(context.Background(), nil, time.Second) return err }, - cleanup: func() { buildBeforeHook = nil }, + cleanup: func() { buildBeforeHook.Reset() }, }, { name: "RunBuildAfterHook", @@ -195,41 +195,41 @@ func TestRunHookErrorPropagation(t *testing.T) { { name: "RunSetupHashreleasePreHook", setup: func(errMsg string) { - setupHashreleasePreHook = func(ctx context.Context, c *cli.Command, dir string) (context.Context, error) { + setupHashreleaseBeforeHook.Add("test-hook", func(ctx context.Context, c *cli.Command, dir string) (context.Context, error) { return ctx, errors.New(errMsg) - } + }) }, run: func() error { - _, err := RunSetupHashreleasePreHook(context.Background(), nil, "/tmp", time.Second) + _, err := RunSetupHashreleaseBeforeHook(context.Background(), nil, "/tmp", time.Second) return err }, - cleanup: func() { setupHashreleasePreHook = nil }, + cleanup: func() { setupHashreleaseBeforeHook.Reset() }, }, { name: "RunPublishBeforeHook", setup: func(errMsg string) { - publishBeforeHook = func(ctx context.Context, c *cli.Command) (context.Context, error) { + publishBeforeHook.Add("test-hook", func(ctx context.Context, c *cli.Command) (context.Context, error) { return ctx, errors.New(errMsg) - } + }) }, run: func() error { _, err := RunPublishBeforeHook(context.Background(), nil, time.Second) return err }, - cleanup: func() { publishBeforeHook = nil }, + cleanup: func() { publishBeforeHook.Reset() }, }, { name: "RunPublishImagePostHook", setup: func(errMsg string) { - publishImagePostHook = func(ctx context.Context, c *cli.Command, published bool) (context.Context, error) { + publishImageAfterHook.Add("test-hook", func(ctx context.Context, c *cli.Command, published bool) (context.Context, error) { return ctx, errors.New(errMsg) - } + }) }, run: func() error { - _, err := RunPublishImagePostHook(context.Background(), nil, true, time.Second) + _, err := RunPublishImageAfterHook(context.Background(), nil, true, time.Second) return err }, - cleanup: func() { publishImagePostHook = nil }, + cleanup: func() { publishImageAfterHook.Reset() }, }, } @@ -268,7 +268,7 @@ func TestRunHooksNilNoOp(t *testing.T) { }) t.Run("RunSetupHashreleasePreHook", func(t *testing.T) { - if rCtx, err := RunSetupHashreleasePreHook(ctx, nil, "/tmp", time.Second); err != nil { + if rCtx, err := RunSetupHashreleaseBeforeHook(ctx, nil, "/tmp", time.Second); err != nil { t.Fatalf("RunSetupHashreleasePreHook with nil hook: %v", err) } else if rCtx != ctx { t.Fatal("RunSetupHashreleasePreHook should return the original context when hook is nil") @@ -284,7 +284,7 @@ func TestRunHooksNilNoOp(t *testing.T) { }) t.Run("RunPublishImagePostHook", func(t *testing.T) { - if rCtx, err := RunPublishImagePostHook(ctx, nil, true, time.Second); err != nil { + if rCtx, err := RunPublishImageAfterHook(ctx, nil, true, time.Second); err != nil { t.Fatalf("RunPostPublishHook with nil hook: %v", err) } else if rCtx != ctx { t.Fatal("RunPostPublishHook should return the original context when hook is nil") diff --git a/hack/release/publish.go b/hack/release/publish.go index 8a1ad1609f..16e8a98a32 100644 --- a/hack/release/publish.go +++ b/hack/release/publish.go @@ -51,11 +51,11 @@ var publishCommand = &cli.Command{ // publishBeforeHook is an optional hook called at the start of publishBefore for additional pre-processing. // It can be set via init() in separate files to extend the publish command behavior. -var publishBeforeHook cliBeforeHookFunc +var publishBeforeHook multiHook[cliBeforeHookFunc] -// publishImagePostHook is an optional hook called after images are published (or skipped). +// publishImageAfterHook is an optional hook called after images are published (or skipped). // It receives whether a new release was published. It can be set via init() in separate files. -var publishImagePostHook imageReleaseHookFunc +var publishImageAfterHook multiHook[imageReleaseHookFunc] // Pre-action for publish command. // It configures logging and performs validations. @@ -119,7 +119,7 @@ var publishAction = cli.ActionFunc(func(ctx context.Context, c *cli.Command) err // Call post-publish hook if registered. hookTimeout := c.Duration(hookTimeoutFlag.Name) - ctx, err = RunPublishImagePostHook(ctx, c, isNewRelease, hookTimeout) + ctx, err = RunPublishImageAfterHook(ctx, c, isNewRelease, hookTimeout) if err != nil { // Post-publish hook errors are non-fatal - images are already published logrus.WithError(err).Warn("Post-publish hook failed (continuing as images are published)") From 0de8b8dd9a8b155c0dc9f133eb910b163d89707d Mon Sep 17 00:00:00 2001 From: tuti Date: Wed, 4 Mar 2026 08:57:58 -0800 Subject: [PATCH 7/8] refactor(release): replace hook system and add allowance for command wrapping - change `setupHashreleaseBuild`, `publishImages` to function variables --- hack/release/build.go | 72 +++++---- hack/release/build_test.go | 77 +++++++++- hack/release/flags.go | 11 +- hack/release/hooks.go | 267 --------------------------------- hack/release/hooks_test.go | 293 ------------------------------------- hack/release/publish.go | 41 +----- 6 files changed, 123 insertions(+), 638 deletions(-) delete mode 100644 hack/release/hooks.go delete mode 100644 hack/release/hooks_test.go diff --git a/hack/release/build.go b/hack/release/build.go index 9c3eed7268..f3c5cab589 100644 --- a/hack/release/build.go +++ b/hack/release/build.go @@ -90,38 +90,25 @@ var buildCommand = &cli.Command{ enterpriseGitBranchFlag, hashreleaseFlag, skipValidationFlag, - hookTimeoutFlag, + extensionTimeoutFlag, }, Before: buildBefore, Action: buildAction, After: buildAfter, } -// buildBeforeHook is an optional hook called at the start of buildBefore for additional pre-processing. -// It can be set via init() in separate files to extend the build command behavior. -var buildBeforeHook multiHook[cliBeforeHookFunc] - -// setupHashreleaseBeforeHook is an optional hook called at the start of setupHashreleaseBuild -// for additional hashrelease setup (e.g., copying image config files). -var setupHashreleaseBeforeHook multiHook[cliHookWithRepoDirFunc] - -// buildAfterHook is an optional hook called in buildAfter for additional post-build tasks. -// This runs regardless of build success or failure, so it can be used for cleanup tasks. -var buildAfterHooks multiHook[cliHookFunc] +// buildCleanupFns collects cleanup functions to run after the build completes (e.g., git reset, temp dir removal). +// Functions are run in reverse order (LIFO) and all errors are collected. +var buildCleanupFns []func(ctx context.Context) error // Pre-action for release build command. var buildBefore = cli.BeforeFunc(func(ctx context.Context, c *cli.Command) (context.Context, error) { configureLogging(c) - // Extract hook timeout once for reuse - hookTimeout := c.Duration(hookTimeoutFlag.Name) + // Start with a clean slate for build cleanup functions. + buildCleanupFns = nil - // Call pre-hook if registered. var err error - ctx, err = RunBuildBeforeHook(ctx, c, hookTimeout) - if err != nil { - return ctx, err - } // Determine build types for Calico and Enterprise if ver := c.String(calicoVersionsConfigFlag.Name); ver != "" { @@ -277,11 +264,28 @@ var buildAction = cli.ActionFunc(func(ctx context.Context, c *cli.Command) error return nil }) +// runBuildCleanup runs all registered cleanup functions in reverse order (LIFO), +// logging each failure individually. It returns the joined errors and resets the slice. +func runBuildCleanup(ctx context.Context) error { + var errs []error + for i := len(buildCleanupFns) - 1; i >= 0; i-- { + if err := buildCleanupFns[i](ctx); err != nil { + logrus.WithError(err).Error("Build cleanup failed") + errs = append(errs, err) + } + } + buildCleanupFns = nil + return errors.Join(errs...) +} + +// buildAfter runs all registered cleanup functions after the build completes. +// Cleanup errors are logged but intentionally not returned to the CLI framework +// as the build result (success or failure) is what matters. var buildAfter = cli.AfterFunc(func(ctx context.Context, c *cli.Command) error { - hookTimeout := c.Duration(hookTimeoutFlag.Name) - if err := RunBuildAfterHook(ctx, c, hookTimeout); err != nil { - // Error should not fail the build, but log it for visibility. - logrus.WithError(err).Error("Build after hook failed") + cleanupCtx, cancel := context.WithTimeout(ctx, c.Duration(extensionTimeoutFlag.Name)) + defer cancel() + if err := runBuildCleanup(cleanupCtx); err != nil { + logrus.WithError(err).Error("One or more build cleanup functions failed") } return nil }) @@ -321,10 +325,10 @@ func assertOperatorImageVersion(registry, image, expectedVersion string) error { return nil } -// Modify component images config and versions for hashrelease builds as needed -// and adds a build after hook to reset any changes to git state after the build completes. -func setupHashreleaseBuild(ctx context.Context, c *cli.Command, repoRootDir string) error { - buildAfterHooks.Add("Reset repo git state", func(ctx context.Context, c *cli.Command) error { +// setupHashreleaseBuild modifies component image config and versions for hashrelease builds. +// It registers a cleanup function to reset git state after the build completes. +var setupHashreleaseBuild = func(ctx context.Context, c *cli.Command, repoRootDir string) error { + buildCleanupFns = append(buildCleanupFns, func(ctx context.Context) error { if out, err := gitInDirContext(ctx, repoRootDir, append([]string{"checkout", "-f"}, changedFiles...)...); err != nil { logrus.Error(out) return fmt.Errorf("resetting git state in repo after hashrelease build: %w", err) @@ -332,13 +336,6 @@ func setupHashreleaseBuild(ctx context.Context, c *cli.Command, repoRootDir stri return nil }) - // Call pre-hook if registered. - var err error - hookTimeout := c.Duration(hookTimeoutFlag.Name) - if ctx, err = RunSetupHashreleaseBeforeHook(ctx, c, repoRootDir, hookTimeout); err != nil { - return fmt.Errorf("setup hashrelease pre-hook failed: %w", err) - } - image := c.String(imageFlag.Name) if image != defaultImageName { imageParts := strings.SplitN(c.String(imageFlag.Name), "/", 2) @@ -504,18 +501,17 @@ func (r *hashreleaseRepo) clone() (string, error) { return "", fmt.Errorf("no git hash found in version %s", r.version) } - // Create a temp directory for cloning the repo. We will clean this up in a build after hook. + // Create a temp directory for cloning the repo. Cleaned up by buildCleanupFns. repoTmpDir, err := os.MkdirTemp("", r.Product+"-*") if err != nil { return "", fmt.Errorf("creating temp directory for %s repo: %w", r.Product, err) } - buildAfterHooks.Add(fmt.Sprintf("Cleaning up %s repo temp directory", r.Product), func(ctx context.Context, c *cli.Command) error { + buildCleanupFns = append(buildCleanupFns, func(ctx context.Context) error { if err := os.RemoveAll(repoTmpDir); err != nil { return fmt.Errorf("removing temp directory %s for %s repo: %w", repoTmpDir, r.Product, err) } return nil - }, - ) + }) remote := "origin" logrus.WithFields(logrus.Fields{ "version": r.version, diff --git a/hack/release/build_test.go b/hack/release/build_test.go index d6b2efb51a..7a20350b95 100644 --- a/hack/release/build_test.go +++ b/hack/release/build_test.go @@ -14,7 +14,13 @@ package main -import "testing" +import ( + "context" + "errors" + "slices" + "strings" + "testing" +) func TestExtractGitHashFromVersion(t *testing.T) { t.Parallel() @@ -81,3 +87,72 @@ func TestExtractGitHashFromVersion(t *testing.T) { }) } } + +// Tests below must NOT be parallel since they mutate package-level vars. + +func TestRunBuildCleanup(t *testing.T) { + t.Run("LIFO order and error collection", func(t *testing.T) { + buildCleanupFns = nil + defer func() { buildCleanupFns = nil }() + + var order []int + buildCleanupFns = append(buildCleanupFns, func(ctx context.Context) error { + order = append(order, 1) + return errors.New("cleanup-1 failed") + }) + buildCleanupFns = append(buildCleanupFns, func(ctx context.Context) error { + order = append(order, 2) + return nil + }) + buildCleanupFns = append(buildCleanupFns, func(ctx context.Context) error { + order = append(order, 3) + return errors.New("cleanup-3 failed") + }) + + err := runBuildCleanup(context.Background()) + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "cleanup-1 failed") { + t.Fatalf("missing cleanup-1 error: %v", err) + } + if !strings.Contains(err.Error(), "cleanup-3 failed") { + t.Fatalf("missing cleanup-3 error: %v", err) + } + if !slices.Equal(order, []int{3, 2, 1}) { + t.Fatalf("expected LIFO order [3, 2, 1], got %v", order) + } + if buildCleanupFns != nil { + t.Fatal("expected buildCleanupFns to be nil after cleanup") + } + }) + + t.Run("empty slice is no-op", func(t *testing.T) { + buildCleanupFns = nil + if err := runBuildCleanup(context.Background()); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("propagates context cancellation", func(t *testing.T) { + buildCleanupFns = nil + defer func() { buildCleanupFns = nil }() + + buildCleanupFns = append(buildCleanupFns, func(ctx context.Context) error { + select { + case <-ctx.Done(): + return ctx.Err() + default: + return nil + } + }) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + err := runBuildCleanup(ctx) + if !errors.Is(err, context.Canceled) { + t.Fatalf("expected context.Canceled, got: %v", err) + } + }) +} + diff --git a/hack/release/flags.go b/hack/release/flags.go index 19bebfbaf3..eeb414ebb3 100644 --- a/hack/release/flags.go +++ b/hack/release/flags.go @@ -23,6 +23,7 @@ import ( "regexp" "slices" "strings" + "time" "github.com/urfave/cli/v3" ) @@ -33,11 +34,11 @@ var debugFlag = &cli.BoolFlag{ Sources: cli.EnvVars("DEBUG"), } -var hookTimeoutFlag = &cli.DurationFlag{ - Name: "hook-timeout", - Usage: "Timeout duration for hook execution", - Sources: cli.EnvVars("HOOK_TIMEOUT"), - Value: DefaultHookTimeout, +var extensionTimeoutFlag = &cli.DurationFlag{ + Name: "timeout", + Usage: "Timeout duration for extension execution", + Sources: cli.EnvVars("EXTENSION_TIMEOUT"), + Value: 5 * time.Minute, } // Git/GitHub related flags. diff --git a/hack/release/hooks.go b/hack/release/hooks.go deleted file mode 100644 index 509fdedbee..0000000000 --- a/hack/release/hooks.go +++ /dev/null @@ -1,267 +0,0 @@ -// Copyright (c) 2026 Tigera, Inc. All rights reserved. - -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package main - -import ( - "context" - "errors" - "fmt" - "slices" - "sync" - "time" - - "github.com/sirupsen/logrus" - "github.com/urfave/cli/v3" -) - -// HOOK SYSTEM DOCUMENTATION -// -// This package implements an extensible hook system that allows external code to extend -// the release build and publish workflows without modifying core logic. -// -// HOOK NAME AND SIGNATURE CONVENTIONS: -// - Workflow hooks are multiHook variables that can have multiple registered hooks. -// See "HOOKS EXECUTION AND ERROR HANDLING" below for execution order and error handling details. -// - Hooks are categorized by the phase of the workflow they run in and follow the naming convention: Hook. For example: -// - buildBeforeHook: runs before the build phase -// - publishImagesAfterHook: runs after the publish images phase -// - Each hook type has a specific function signature that defines the parameters it receives and if it can modify the context for subsequent actions. For example: -// - cliHookFunc does not modify the context: func(ctx context.Context, c *cli.Command) error -// - cliBeforeHookFunc can modify the context: func(ctx context.Context, c *cli.Command) (context.Context, error) -// -// HOOK REGISTRATION: -// -// Hooks are registered by calling the Add method on the appropriate multiHook variable, typically from an init() function. For example: -// -// // myhooks.go -// func init() { -// buildBeforeHook.Add("my custom before hook", myCustomBuildBeforeHook) -// buildAfterHook.Add("my custom after hook", myCustomBuildAfterHook) -// } -// -// HOOK EXECUTION AND ERROR HANDLING: -// -// - Hooks are executed with a derived context that includes the main operation's context -// - Hooks that run in the "Before" phase (e.g., buildBeforeHook) run in registration order and stop on the first error (fail-fast). -// - Hooks that run in the "After" phase (e.g., buildAfterHook) run in reverse registration order and collect all errors, returning them as a single error. -// - If a hook times out, the error message clearly indicates which hook timed out -// - If a hook returns an error, it is wrapped with context about which hook failed -// - If a hook panics, the panic is NOT caught - letting it propagate to the caller -// - Timeout values can be configured via the --hook-timeout flag, -// and should be set based on expected hook execution time and overall operation time budget -// -// TIMEOUT CONFIGURATION: -// -// Timeout is configured via the --hook-timeout flag (default: 5 minutes). - -// DefaultHookTimeout is the default timeout for all hooks (5 minutes). -const DefaultHookTimeout = 5 * time.Minute - -// cliHookFunc is the signature for general purpose hooks that run during command execution. -type cliHookFunc func(ctx context.Context, c *cli.Command) error - -// cliBeforeHookFunc is the signature for hooks that run in the cli.BeforeFunc phase and can modify the context for subsequent actions. -type cliBeforeHookFunc func(ctx context.Context, c *cli.Command) (context.Context, error) - -// cliHookWithRepoDirFunc is the signature for hooks that require access to the repository root directory (e.g., hashrelease setup). -type cliHookWithRepoDirFunc func(ctx context.Context, c *cli.Command, repoRootDir string) (context.Context, error) - -// imageReleaseHookFunc is the signature for hooks that run in publish action after images are published (or skipped). -// It receives information about whether the release was newly published, allowing it to perform actions accordingly. -type imageReleaseHookFunc func(ctx context.Context, c *cli.Command, published bool) (context.Context, error) - -type cliHook[T any] struct { - Desc string - Hook T -} - -type multiHook[T any] struct { - mu sync.Mutex - hooks []cliHook[T] -} - -func (h *multiHook[T]) Add(desc string, hook T) { - h.mu.Lock() - defer h.mu.Unlock() - h.hooks = append(h.hooks, cliHook[T]{ - Desc: desc, - Hook: hook, - }) -} - -func (h *multiHook[T]) Hooks() []cliHook[T] { - h.mu.Lock() - defer h.mu.Unlock() - return slices.Clone(h.hooks) -} - -func (h *multiHook[T]) Reset() { - h.mu.Lock() - defer h.mu.Unlock() - h.hooks = nil -} - -type withTimeoutResult[T any] struct { - value T - err error -} - -// withTimeout is a generic helper that executes a function with timeout and error handling. -// It distinguishes between timeout and other errors and provides clear error messages. -// If the timeout is <= 0, it will use the DefaultHookTimeout. -// -// The hook function receives a cancellable context derived from the parent. On timeout or -// parent cancellation, this context is cancelled to signal the hook to stop. On success, -// the context is intentionally not cancelled so that any context the hook returns (which -// may be derived from it) remains valid for the caller. -// -// Hook functions should respect context cancellation (e.g., use context-aware I/O) -// to ensure the goroutine exits promptly when a timeout or cancellation occurs. -func withTimeout[T any](ctx context.Context, timeout time.Duration, hookName string, fn func(context.Context) (T, error)) (T, error) { - zero := new(T) // zero value for type T - if fn == nil { - return *zero, nil - } - if timeout <= 0 { - timeout = DefaultHookTimeout - } - logrus.WithFields(logrus.Fields{ - "hook": hookName, - "timeout": timeout, - }).Debug("Running hook") - - hookCtx, hookCancel := context.WithCancel(ctx) - // Cancel the hook context on any non-success path to signal the hook goroutine to stop. - // On success, hookCancel is NOT called because the returned value may be a context - // derived from hookCtx, and cancelling it would invalidate the caller's context. - succeeded := false - defer func() { - if !succeeded { - hookCancel() - } - }() - - timer := time.NewTimer(timeout) - done := make(chan withTimeoutResult[T], 1) - go func() { - value, err := fn(hookCtx) - done <- withTimeoutResult[T]{value, err} - }() - - select { - case r := <-done: - timer.Stop() - if r.err != nil { - return r.value, fmt.Errorf("%s failed: %w", hookName, r.err) - } - succeeded = true - return r.value, nil - case <-timer.C: - return *zero, fmt.Errorf("%s timed out after %v", hookName, timeout) - case <-ctx.Done(): - timer.Stop() - return *zero, fmt.Errorf("%s: parent context cancelled: %w", hookName, ctx.Err()) - } -} - -// runHooksFailFast runs all hooks in registration order, stopping on the first error. -func runHooksFailFast[T any]( - ctx context.Context, - timeout time.Duration, - hookName string, - mh *multiHook[T], - run func(T, context.Context) (context.Context, error), -) (context.Context, error) { - hooks := mh.Hooks() - if len(hooks) == 0 { - return ctx, nil - } - return withTimeout(ctx, timeout, hookName, func(hookCtx context.Context) (context.Context, error) { - for _, h := range hooks { - var err error - hookCtx, err = run(h.Hook, hookCtx) - if err != nil { - return hookCtx, fmt.Errorf("%s %q failed: %w", hookName, h.Desc, err) - } - } - return hookCtx, nil - }) -} - -// runHooksCollectErrors runs all hooks in reverse (LIFO) order, collecting all errors. -func runHooksCollectErrors[T any]( - ctx context.Context, - timeout time.Duration, - hookName string, - mh *multiHook[T], - run func(T, context.Context) (context.Context, error), -) (context.Context, error) { - hooks := mh.Hooks() - if len(hooks) == 0 { - return ctx, nil - } - return withTimeout(ctx, timeout, hookName, func(hookCtx context.Context) (context.Context, error) { - var errs []error - for i := len(hooks) - 1; i >= 0; i-- { - var err error - hookCtx, err = run(hooks[i].Hook, hookCtx) - if err != nil { - errs = append(errs, fmt.Errorf("%s %q failed: %w", hookName, hooks[i].Desc, err)) - } - } - return hookCtx, errors.Join(errs...) - }) -} - -// RunBuildBeforeHook executes the buildBeforeHook with timeout and error handling. -func RunBuildBeforeHook(ctx context.Context, c *cli.Command, timeout time.Duration) (context.Context, error) { - return runHooksFailFast(ctx, timeout, "buildBeforeHook", &buildBeforeHook, - func(h cliBeforeHookFunc, ctx context.Context) (context.Context, error) { - return h(ctx, c) - }) -} - -// RunBuildAfterHook executes the buildAfterHook with timeout and error handling. -func RunBuildAfterHook(ctx context.Context, c *cli.Command, timeout time.Duration) error { - _, err := runHooksCollectErrors(ctx, timeout, "buildAfterHook", &buildAfterHooks, - func(h cliHookFunc, ctx context.Context) (context.Context, error) { - return ctx, h(ctx, c) - }) - return err -} - -// RunSetupHashreleaseBeforeHook executes the setupHashreleaseBeforeHook with timeout and error handling. -func RunSetupHashreleaseBeforeHook(ctx context.Context, c *cli.Command, repoRootDir string, timeout time.Duration) (context.Context, error) { - return runHooksFailFast(ctx, timeout, "setupHashreleaseBeforeHook", &setupHashreleaseBeforeHook, - func(h cliHookWithRepoDirFunc, ctx context.Context) (context.Context, error) { - return h(ctx, c, repoRootDir) - }) -} - -// RunPublishBeforeHook executes the publishBeforeHook with timeout and error handling. -func RunPublishBeforeHook(ctx context.Context, c *cli.Command, timeout time.Duration) (context.Context, error) { - return runHooksFailFast(ctx, timeout, "publishBeforeHook", &publishBeforeHook, - func(h cliBeforeHookFunc, ctx context.Context) (context.Context, error) { - return h(ctx, c) - }) -} - -// RunPublishImageAfterHook executes the publishImageAfterHook with timeout and error handling. -func RunPublishImageAfterHook(ctx context.Context, c *cli.Command, published bool, timeout time.Duration) (context.Context, error) { - return runHooksCollectErrors(ctx, timeout, "publishImageAfterHook", &publishImageAfterHook, - func(h imageReleaseHookFunc, ctx context.Context) (context.Context, error) { - return h(ctx, c, published) - }) -} diff --git a/hack/release/hooks_test.go b/hack/release/hooks_test.go deleted file mode 100644 index a2be270b52..0000000000 --- a/hack/release/hooks_test.go +++ /dev/null @@ -1,293 +0,0 @@ -// Copyright (c) 2026 Tigera, Inc. All rights reserved. - -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package main - -import ( - "context" - "errors" - "strings" - "testing" - "testing/synctest" - "time" - - "github.com/urfave/cli/v3" -) - -func TestWithTimeout(t *testing.T) { - t.Parallel() - - t.Run("nil function returns zero value", func(t *testing.T) { - t.Parallel() - val, err := withTimeout[string](context.Background(), time.Second, "test-hook", nil) - if err != nil { - t.Fatalf("expected no error, got: %v", err) - } - if val != "" { - t.Fatalf("expected empty string, got: %q", val) - } - }) - - t.Run("successful execution", func(t *testing.T) { - t.Parallel() - val, err := withTimeout(context.Background(), time.Second, "test-hook", func(ctx context.Context) (string, error) { - return "result", nil - }) - if err != nil { - t.Fatalf("expected no error, got: %v", err) - } - if val != "result" { - t.Fatalf("expected %q, got: %q", "result", val) - } - }) - - t.Run("function returns error", func(t *testing.T) { - t.Parallel() - _, err := withTimeout(context.Background(), time.Second, "test-hook", func(ctx context.Context) (string, error) { - return "", errors.New("hook error") - }) - if err == nil { - t.Fatal("expected error, got nil") - } - if !strings.Contains(err.Error(), "test-hook failed") { - t.Fatalf("expected error to contain 'test-hook failed', got: %v", err) - } - if !strings.Contains(err.Error(), "hook error") { - t.Fatalf("expected error to contain 'hook error', got: %v", err) - } - }) - - t.Run("timeout triggers and cancels hook context", func(t *testing.T) { - t.Parallel() - synctest.Test(t, func(t *testing.T) { - _, err := withTimeout(context.Background(), 5*time.Minute, "slow-hook", func(ctx context.Context) (string, error) { - <-ctx.Done() - return "", ctx.Err() - }) - if err == nil { - t.Fatal("expected timeout error, got nil") - } - if !strings.Contains(err.Error(), "slow-hook timed out") { - t.Fatalf("expected timeout error message, got: %v", err) - } - }) - }) - - t.Run("parent context cancelled cancels hook context", func(t *testing.T) { - t.Parallel() - synctest.Test(t, func(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - go func() { - time.Sleep(1 * time.Minute) - cancel() - }() - _, err := withTimeout(ctx, 5*time.Minute, "cancel-hook", func(ctx context.Context) (string, error) { - // Hook respects context cancellation, so it exits when the parent is cancelled. - <-ctx.Done() - return "", ctx.Err() - }) - if err == nil { - t.Fatal("expected error, got nil") - } - if !strings.Contains(err.Error(), "parent context cancelled") { - t.Fatalf("expected parent context cancelled error, got: %v", err) - } - }) - }) - - t.Run("zero timeout uses default", func(t *testing.T) { - t.Parallel() - val, err := withTimeout(context.Background(), 0, "test-hook", func(ctx context.Context) (int, error) { - return 42, nil - }) - if err != nil { - t.Fatalf("expected no error, got: %v", err) - } - if val != 42 { - t.Fatalf("expected 42, got: %d", val) - } - }) -} - -// Run* hook tests must NOT be parallel since it uses package-level hook vars that can be mutated by other tests. -func TestRunBuildAfterHookRunsAll(t *testing.T) { - defer buildAfterHooks.Reset() - - var ran []string - buildAfterHooks.Add("hook-1", func(ctx context.Context, c *cli.Command) error { - ran = append(ran, "hook-1") - return errors.New("hook-1 error") - }) - buildAfterHooks.Add("hook-2", func(ctx context.Context, c *cli.Command) error { - ran = append(ran, "hook-2") - return nil - }) - buildAfterHooks.Add("hook-3", func(ctx context.Context, c *cli.Command) error { - ran = append(ran, "hook-3") - return errors.New("hook-3 error") - }) - - err := RunBuildAfterHook(context.Background(), nil, 5*time.Second) - // Should have errors from hook-1 and hook-3 - if err == nil { - t.Fatal("expected error, got nil") - } - if !strings.Contains(err.Error(), "hook-1 error") { - t.Fatalf("expected hook-1 error in result, got: %v", err) - } - if !strings.Contains(err.Error(), "hook-3 error") { - t.Fatalf("expected hook-3 error in result, got: %v", err) - } - // All three hooks should have run (in LIFO order: hook-3, hook-2, hook-1) - if len(ran) != 3 { - t.Fatalf("expected all 3 hooks to run, got %d: %v", len(ran), ran) - } - if ran[0] != "hook-3" || ran[1] != "hook-2" || ran[2] != "hook-1" { - t.Fatalf("expected LIFO order [hook-3, hook-2, hook-1], got %v", ran) - } -} - -// Run* hook tests must NOT be parallel since it uses package-level hook vars that can be mutated by other tests. -func TestRunHookErrorPropagation(t *testing.T) { - cases := []struct { - name string - setup func(errMsg string) - run func() error - cleanup func() - }{ - { - name: "RunBuildBeforeHook", - setup: func(errMsg string) { - buildBeforeHook.Add("test-hook", func(ctx context.Context, c *cli.Command) (context.Context, error) { - return ctx, errors.New(errMsg) - }) - }, - run: func() error { - _, err := RunBuildBeforeHook(context.Background(), nil, time.Second) - return err - }, - cleanup: func() { buildBeforeHook.Reset() }, - }, - { - name: "RunBuildAfterHook", - setup: func(errMsg string) { - buildAfterHooks.Add("test-hook", func(ctx context.Context, c *cli.Command) error { - return errors.New(errMsg) - }, - ) - }, - run: func() error { - return RunBuildAfterHook(context.Background(), nil, time.Second) - }, - cleanup: func() { buildAfterHooks.Reset() }, - }, - { - name: "RunSetupHashreleasePreHook", - setup: func(errMsg string) { - setupHashreleaseBeforeHook.Add("test-hook", func(ctx context.Context, c *cli.Command, dir string) (context.Context, error) { - return ctx, errors.New(errMsg) - }) - }, - run: func() error { - _, err := RunSetupHashreleaseBeforeHook(context.Background(), nil, "/tmp", time.Second) - return err - }, - cleanup: func() { setupHashreleaseBeforeHook.Reset() }, - }, - { - name: "RunPublishBeforeHook", - setup: func(errMsg string) { - publishBeforeHook.Add("test-hook", func(ctx context.Context, c *cli.Command) (context.Context, error) { - return ctx, errors.New(errMsg) - }) - }, - run: func() error { - _, err := RunPublishBeforeHook(context.Background(), nil, time.Second) - return err - }, - cleanup: func() { publishBeforeHook.Reset() }, - }, - { - name: "RunPublishImagePostHook", - setup: func(errMsg string) { - publishImageAfterHook.Add("test-hook", func(ctx context.Context, c *cli.Command, published bool) (context.Context, error) { - return ctx, errors.New(errMsg) - }) - }, - run: func() error { - _, err := RunPublishImageAfterHook(context.Background(), nil, true, time.Second) - return err - }, - cleanup: func() { publishImageAfterHook.Reset() }, - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - defer tc.cleanup() - errMsg := tc.name + " failed" - tc.setup(errMsg) - err := tc.run() - if err == nil { - t.Fatal("expected error, got nil") - } - if !strings.Contains(err.Error(), errMsg) { - t.Fatalf("expected error to contain %q, got: %v", errMsg, err) - } - }) - } -} - -// Run* hook tests must NOT be parallel since it uses package-level hook vars that can be mutated by other tests. -func TestRunHooksNilNoOp(t *testing.T) { - ctx := context.Background() - - t.Run("RunBuildBeforeHook", func(t *testing.T) { - if rCtx, err := RunBuildBeforeHook(ctx, nil, time.Second); err != nil { - t.Fatalf("RunBuildPreHook with nil hook: %v", err) - } else if rCtx != ctx { - t.Fatal("RunBuildPreHook should return the original context when hook is nil") - } - }) - - t.Run("RunBuildAfterHook", func(t *testing.T) { - if err := RunBuildAfterHook(ctx, nil, time.Second); err != nil { - t.Fatalf("RunBuildAfterHook with nil hook: %v", err) - } - }) - - t.Run("RunSetupHashreleasePreHook", func(t *testing.T) { - if rCtx, err := RunSetupHashreleaseBeforeHook(ctx, nil, "/tmp", time.Second); err != nil { - t.Fatalf("RunSetupHashreleasePreHook with nil hook: %v", err) - } else if rCtx != ctx { - t.Fatal("RunSetupHashreleasePreHook should return the original context when hook is nil") - } - }) - - t.Run("RunPublishBeforeHook", func(t *testing.T) { - if rCtx, err := RunPublishBeforeHook(ctx, nil, time.Second); err != nil { - t.Fatalf("RunPublishPreHook with nil hook: %v", err) - } else if rCtx != ctx { - t.Fatal("RunPublishPreHook should return the original context when hook is nil") - } - }) - - t.Run("RunPublishImagePostHook", func(t *testing.T) { - if rCtx, err := RunPublishImageAfterHook(ctx, nil, true, time.Second); err != nil { - t.Fatalf("RunPostPublishHook with nil hook: %v", err) - } else if rCtx != ctx { - t.Fatal("RunPostPublishHook should return the original context when hook is nil") - } - }) -} diff --git a/hack/release/publish.go b/hack/release/publish.go index 16e8a98a32..97594d890e 100644 --- a/hack/release/publish.go +++ b/hack/release/publish.go @@ -43,34 +43,17 @@ var publishCommand = &cli.Command{ createGithubReleaseFlag, githubTokenFlag, draftGithubReleaseFlag, - hookTimeoutFlag, }, Before: publishBefore, Action: publishAction, } -// publishBeforeHook is an optional hook called at the start of publishBefore for additional pre-processing. -// It can be set via init() in separate files to extend the publish command behavior. -var publishBeforeHook multiHook[cliBeforeHookFunc] - -// publishImageAfterHook is an optional hook called after images are published (or skipped). -// It receives whether a new release was published. It can be set via init() in separate files. -var publishImageAfterHook multiHook[imageReleaseHookFunc] - // Pre-action for publish command. // It configures logging and performs validations. var publishBefore = cli.BeforeFunc(func(ctx context.Context, c *cli.Command) (context.Context, error) { configureLogging(c) - // Extract hook timeout once for reuse - hookTimeout := c.Duration(hookTimeoutFlag.Name) - - // Call pre-hook if registered. var err error - ctx, err = RunPublishBeforeHook(ctx, c, hookTimeout) - if err != nil { - return ctx, err - } ctx, err = addRepoInfoToCtx(ctx, c.String(gitRepoFlag.Name)) if err != nil { @@ -112,19 +95,10 @@ var publishAction = cli.ActionFunc(func(ctx context.Context, c *cli.Command) err } // Publish images - isNewRelease, err := publishImages(c, repoRootDir) - if err != nil { + if err := publishImages(c, repoRootDir); err != nil { return err } - // Call post-publish hook if registered. - hookTimeout := c.Duration(hookTimeoutFlag.Name) - ctx, err = RunPublishImageAfterHook(ctx, c, isNewRelease, hookTimeout) - if err != nil { - // Post-publish hook errors are non-fatal - images are already published - logrus.WithError(err).Warn("Post-publish hook failed (continuing as images are published)") - } - // Only images are published for hashrelease builds. if c.Bool(hashreleaseFlag.Name) { return nil @@ -138,18 +112,17 @@ var publishAction = cli.ActionFunc(func(ctx context.Context, c *cli.Command) err return publishGithubRelease(ctx, c, repoRootDir) }) -// Publish the operator images to the specified registry. +// publishImages publishes the operator images to the specified registry. // If the images are already published, it skips publishing. -// Returns true if images were newly published, false if they already existed. -func publishImages(c *cli.Command, repoRootDir string) (bool, error) { +var publishImages = func(c *cli.Command, repoRootDir string) error { version := c.String(versionFlag.Name) log := logrus.WithField("version", version) // Check if images are already published if published, err := operatorImagePublished(c); err != nil { - return false, fmt.Errorf("checking if images are already published: %w", err) + return fmt.Errorf("checking if images are already published: %w", err) } else if published { log.Warn("Images are already published") - return false, nil + return nil } // Set up environment variables for publish @@ -182,10 +155,10 @@ func publishImages(c *cli.Command, repoRootDir string) (bool, error) { log.Info("Publishing Operator images") if out, err := makeInDir(repoRootDir, "release-publish-images", publishEnv...); err != nil { log.Error(out) - return false, fmt.Errorf("publishing images: %w", err) + return fmt.Errorf("publishing images: %w", err) } log.Info("Successfully published Operator images") - return true, nil + return nil } // Check if the operator image is already published. From 1244c2bc559b377118eed9fdc85725c928c246c1 Mon Sep 17 00:00:00 2001 From: tuti Date: Wed, 4 Mar 2026 09:48:41 -0800 Subject: [PATCH 8/8] fix(release): cleanups - remove running command in context: only added as part of hook system - add to buildCleanupFn for hashreleases in cli.Action --- hack/release/build.go | 15 +++++++------ hack/release/build_test.go | 1 - hack/release/utils.go | 43 -------------------------------------- hack/release/utils_test.go | 26 ----------------------- 4 files changed, 7 insertions(+), 78 deletions(-) diff --git a/hack/release/build.go b/hack/release/build.go index f3c5cab589..0010129c2f 100644 --- a/hack/release/build.go +++ b/hack/release/build.go @@ -243,6 +243,13 @@ var buildAction = cli.ActionFunc(func(ctx context.Context, c *cli.Command) error if c.Bool(hashreleaseFlag.Name) { buildLog = buildLog.WithField("hashrelease", true) buildEnv = append(buildEnv, fmt.Sprintf("GIT_VERSION=%s", c.String(versionFlag.Name))) + buildCleanupFns = append(buildCleanupFns, func(ctx context.Context) error { + if out, err := gitInDir(repoRootDir, append([]string{"checkout", "-f"}, changedFiles...)...); err != nil { + logrus.Error(out) + return fmt.Errorf("resetting git state in repo after hashrelease build: %w", err) + } + return nil + }) if err := setupHashreleaseBuild(ctx, c, repoRootDir); err != nil { return fmt.Errorf("preparing hashrelease build environment: %w", err) } @@ -328,14 +335,6 @@ func assertOperatorImageVersion(registry, image, expectedVersion string) error { // setupHashreleaseBuild modifies component image config and versions for hashrelease builds. // It registers a cleanup function to reset git state after the build completes. var setupHashreleaseBuild = func(ctx context.Context, c *cli.Command, repoRootDir string) error { - buildCleanupFns = append(buildCleanupFns, func(ctx context.Context) error { - if out, err := gitInDirContext(ctx, repoRootDir, append([]string{"checkout", "-f"}, changedFiles...)...); err != nil { - logrus.Error(out) - return fmt.Errorf("resetting git state in repo after hashrelease build: %w", err) - } - return nil - }) - image := c.String(imageFlag.Name) if image != defaultImageName { imageParts := strings.SplitN(c.String(imageFlag.Name), "/", 2) diff --git a/hack/release/build_test.go b/hack/release/build_test.go index 7a20350b95..802672bebf 100644 --- a/hack/release/build_test.go +++ b/hack/release/build_test.go @@ -155,4 +155,3 @@ func TestRunBuildCleanup(t *testing.T) { } }) } - diff --git a/hack/release/utils.go b/hack/release/utils.go index 9c234237b8..7d2daf235d 100644 --- a/hack/release/utils.go +++ b/hack/release/utils.go @@ -25,7 +25,6 @@ import ( "path/filepath" "regexp" "strings" - "syscall" "github.com/Masterminds/semver/v3" "github.com/sirupsen/logrus" @@ -133,48 +132,6 @@ func runCommandInDir(dir, name string, args, env []string) (string, error) { return strings.TrimSpace(outb.String()), err } -// runCommandInDirContext is a context-aware variant of runCommandInDir. -// When ctx is cancelled, the entire process group is killed to ensure child processes are cleaned up. -func runCommandInDirContext(ctx context.Context, dir, name string, args, env []string) (string, error) { - cmd := exec.CommandContext(ctx, name, args...) - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} - cmd.Cancel = func() error { - if cmd.Process == nil { - return nil - } - return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) - } - if len(env) != 0 { - cmd.Env = env - } - cmd.Dir = dir - var outb, errb bytes.Buffer - if logrus.IsLevelEnabled(logrus.DebugLevel) { - cmd.Stdout = io.MultiWriter(os.Stdout, &outb) - cmd.Stderr = io.MultiWriter(os.Stderr, &errb) - } else { - cmd.Stdout = io.MultiWriter(&outb) - cmd.Stderr = io.MultiWriter(&errb) - } - logrus.WithFields(logrus.Fields{ - "cmd": cmd.String(), - "dir": dir, - }).Debugf("Running %s command (context-aware)", name) - err := cmd.Run() - if err != nil { - errDesc := fmt.Sprintf(`running command "%s %s"`, name, strings.Join(args, " ")) - if dir != "" { - errDesc += fmt.Sprintf(" in directory %s", dir) - } - err = fmt.Errorf("%s: %w \n%s", errDesc, err, strings.TrimSpace(errb.String())) - } - return strings.TrimSpace(outb.String()), err -} - -func gitInDirContext(ctx context.Context, dir string, args ...string) (string, error) { - return runCommandInDirContext(ctx, dir, "git", args, nil) -} - func addRepoInfoToCtx(ctx context.Context, repo string) (context.Context, error) { if ctx.Value(githubOrgCtxKey) != nil && ctx.Value(githubRepoCtxKey) != nil { return ctx, nil diff --git a/hack/release/utils_test.go b/hack/release/utils_test.go index 991cac37fc..e0843a715b 100644 --- a/hack/release/utils_test.go +++ b/hack/release/utils_test.go @@ -15,7 +15,6 @@ package main import ( - "context" "fmt" "os" "path/filepath" @@ -312,31 +311,6 @@ func TestIsValidReleaseVersionOverride(t *testing.T) { }) } -func TestRunCommandInDirContext(t *testing.T) { - t.Parallel() - - t.Run("successful execution", func(t *testing.T) { - t.Parallel() - out, err := runCommandInDirContext(context.Background(), "", "echo", []string{"hello"}, nil) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if out != "hello" { - t.Fatalf("expected %q, got %q", "hello", out) - } - }) - - t.Run("context cancellation kills process", func(t *testing.T) { - t.Parallel() - ctx, cancel := context.WithCancel(context.Background()) - cancel() // cancel immediately - _, err := runCommandInDirContext(ctx, "", "sleep", []string{"60"}, nil) - if err == nil { - t.Fatal("expected error from cancelled context, got nil") - } - }) -} - func TestIsPrereleaseEnterpriseVersion(t *testing.T) { t.Parallel()