From 3d4ccb1137737a30e8646844a9e3eec87a12e30a Mon Sep 17 00:00:00 2001 From: Kent Rancourt Date: Mon, 16 Dec 2024 13:14:19 -0500 Subject: [PATCH] make max attempts cofigurable Signed-off-by: Kent Rancourt --- docs/docs/35-references/10-promotion-steps.md | 20 +++++++++++++- internal/directives/git_pusher.go | 27 ++++++++++++------- internal/directives/git_pusher_test.go | 19 +++++++++++++ .../directives/schemas/git-push-config.json | 6 +++++ internal/directives/zz_config_types.go | 6 +++++ ui/src/gen/directives/git-push-config.json | 6 +++++ 6 files changed, 74 insertions(+), 10 deletions(-) diff --git a/docs/docs/35-references/10-promotion-steps.md b/docs/docs/35-references/10-promotion-steps.md index f168c38cc..df9a15b94 100644 --- a/docs/docs/35-references/10-promotion-steps.md +++ b/docs/docs/35-references/10-promotion-steps.md @@ -924,7 +924,24 @@ steps: ### `git-push` `git-push` pushes the committed changes in a specified working tree to a -specified branch in the remote repository. This step typically follows a `git-commit` step and is often followed by a `git-open-pr` step. +specified branch in the remote repository. This step typically follows a +`git-commit` step and is often followed by a `git-open-pr` step. + +This step also implements its own, internal retry logic. If a push fails, with +the cause determined to be the presence of new commits in the remote branch that +are not present in the local branch, the step will attempt to rebase before +retrying the push. Any merge conflict requiring manual resolution will +immediately halt further attempts. + +:::info +This step's internal retry logic is helpful in scenarios when concurrent +Promotions to multiple Stages may all write to the same branch of the same +repository. + +Because conflicts requiring manual resolution will halt further attempts, it is +recommended to design your Promotion processes such that Promotions to multiple +Stages that write to the same branch do not write to the same files. +::: #### `git-push` Configuration @@ -932,6 +949,7 @@ specified branch in the remote repository. This step typically follows a `git-co |------|------|----------|-------------| | `path` | `string` | Y | Path to a Git working tree containing committed changes. | | `targetBranch` | `string` | N | The branch to push to in the remote repository. Mutually exclusive with `generateTargetBranch=true`. If neither of these is provided, the target branch will be the same as the branch currently checked out in the working tree. | +| `maxAttempts` | `int32` | N | The maximum number of attempts to make when pushing to the remote repository. Default is 50. | | `generateTargetBranch` | `boolean` | N | Whether to push to a remote branch named like `kargo///promotion`. If such a branch does not already exist, it will be created. A value of 'true' is mutually exclusive with `targetBranch`. If neither of these is provided, the target branch will be the currently checked out branch. This option is useful when a subsequent promotion step will open a pull request against a Stage-specific branch. In such a case, the generated target branch pushed to by the `git-push` step can later be utilized as the source branch of the pull request. | #### `git-push` Examples diff --git a/internal/directives/git_pusher.go b/internal/directives/git_pusher.go index 8e2b1aee8..04d47f0bf 100644 --- a/internal/directives/git_pusher.go +++ b/internal/directives/git_pusher.go @@ -3,11 +3,9 @@ package directives import ( "context" "fmt" - "time" securejoin "github.com/cyphar/filepath-securejoin" "github.com/xeipuuv/gojsonschema" - "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/util/retry" kargoapi "github.com/akuity/kargo/api/v1alpha1" @@ -131,14 +129,25 @@ func (g *gitPushPusher) runPromotionStep( } } + backoff := retry.DefaultBackoff + if cfg.MaxAttempts != nil { + // Note, the docs for this field say: + // + // The remaining number of iterations in which the duration + // parameter may change... + // + // This is misleading, as it implies that the total number of attempts may + // exceed the value of Steps and that Steps only dictates the maximum number + // of adjustments to the interval between retries. + // + // Reading the implementation of retry.DefaultBackoff reveals that Steps + // is indeed the maximum number of attempts. + backoff.Steps = int(*cfg.MaxAttempts) + } else { + backoff.Steps = 50 + } if err = retry.OnError( - wait.Backoff{ // TODO(krancour): Make this at least partially configurable - Duration: 1 * time.Second, - Factor: 2, - Steps: 10, - Cap: 2 * time.Minute, - Jitter: 0.1, - }, + backoff, git.IsNonFastForward, func() error { return workTree.Push(pushOpts) diff --git a/internal/directives/git_pusher_test.go b/internal/directives/git_pusher_test.go index 7b5618c64..f128de166 100644 --- a/internal/directives/git_pusher_test.go +++ b/internal/directives/git_pusher_test.go @@ -3,6 +3,7 @@ package directives import ( "context" "fmt" + "math" "net/http/httptest" "os" "path/filepath" @@ -37,6 +38,24 @@ func Test_gitPusher_validate(t *testing.T) { "path: String length must be greater than or equal to 1", }, }, + { + name: "maxAttempts < 1", + config: Config{ + "maxAttempts": 0, + }, + expectedProblems: []string{ + "maxAttempts: Must be greater than or equal to 1", + }, + }, + { + name: fmt.Sprintf("maxAttempts > %d", math.MaxInt32), + config: Config{ + "maxAttempts": math.MaxInt32 + 1, + }, + expectedProblems: []string{ + fmt.Sprintf("maxAttempts: Must be less than or equal to %.9e", float64(math.MaxInt32)), + }, + }, { name: "just generateTargetBranch is true", config: Config{ // Should be completely valid diff --git a/internal/directives/schemas/git-push-config.json b/internal/directives/schemas/git-push-config.json index 7d1266109..fd18ff5a1 100644 --- a/internal/directives/schemas/git-push-config.json +++ b/internal/directives/schemas/git-push-config.json @@ -9,6 +9,12 @@ "type": "boolean", "description": "Indicates whether to push to a new remote branch. A value of 'true' is mutually exclusive with 'targetBranch'. If neither of these is provided, the target branch will be the currently checked out branch." }, + "maxAttempts": { + "type": "integer", + "description": "This step implements its own internal retry logic for cases where a push is determined to have failed due to the remote branch having commits that that are not present locally. Each attempt, including the first, rebases prior to pushing. This field configures the maximum number of attempts to push to the remote repository. If not specified, the default is 50.", + "minimum": 1, + "maximum": 2147483647 + }, "path": { "type": "string", "description": "The path to a working directory of a local repository.", diff --git a/internal/directives/zz_config_types.go b/internal/directives/zz_config_types.go index e09b38c60..5ffe2e456 100644 --- a/internal/directives/zz_config_types.go +++ b/internal/directives/zz_config_types.go @@ -211,6 +211,12 @@ type GitPushConfig struct { // with 'targetBranch'. If neither of these is provided, the target branch will be the // currently checked out branch. GenerateTargetBranch bool `json:"generateTargetBranch,omitempty"` + // This step implements its own internal retry logic for cases where a push is determined to + // have failed due to the remote branch having commits that that are not present locally. + // Each attempt, including the first, rebases prior to pushing. This field configures the + // maximum number of attempts to push to the remote repository. If not specified, the + // default is 50. + MaxAttempts *int64 `json:"maxAttempts,omitempty"` // The path to a working directory of a local repository. Path string `json:"path"` // The target branch to push to. Mutually exclusive with 'generateTargetBranch=true'. If diff --git a/ui/src/gen/directives/git-push-config.json b/ui/src/gen/directives/git-push-config.json index 1532cfe35..903e62dd4 100644 --- a/ui/src/gen/directives/git-push-config.json +++ b/ui/src/gen/directives/git-push-config.json @@ -8,6 +8,12 @@ "type": "boolean", "description": "Indicates whether to push to a new remote branch. A value of 'true' is mutually exclusive with 'targetBranch'. If neither of these is provided, the target branch will be the currently checked out branch." }, + "maxAttempts": { + "type": "integer", + "description": "This step implements its own internal retry logic for cases where a push is determined to have failed due to the remote branch having commits that that are not present locally. Each attempt, including the first, rebases prior to pushing. This field configures the maximum number of attempts to push to the remote repository. If not specified, the default is 50.", + "minimum": 1, + "maximum": 2147483647 + }, "path": { "type": "string", "description": "The path to a working directory of a local repository.",