From ce44d5bebd3b631606687bd2795c2e58a0736897 Mon Sep 17 00:00:00 2001 From: Taras Postument Date: Tue, 17 Dec 2024 17:19:32 +0000 Subject: [PATCH] PR feedback, part 2 --- tools/amplify-preview/amplify.go | 57 ++++++++++++++++++++++---------- tools/amplify-preview/github.go | 17 +++++----- tools/amplify-preview/main.go | 50 +++++++++++++++++++--------- 3 files changed, 81 insertions(+), 43 deletions(-) diff --git a/tools/amplify-preview/amplify.go b/tools/amplify-preview/amplify.go index f5573bd..4d499a9 100644 --- a/tools/amplify-preview/amplify.go +++ b/tools/amplify-preview/amplify.go @@ -20,6 +20,7 @@ import ( "context" "errors" "fmt" + "log/slog" "strconv" "strings" "sync" @@ -32,13 +33,9 @@ import ( ) var ( - errBranchNotFound = errors.New("branch not found") - errNoJobForBranch = errors.New("current branch has no jobs") - amplifyJobCompletedStatuses = map[types.JobStatus]struct{}{ - types.JobStatusFailed: {}, - types.JobStatusCancelled: {}, - types.JobStatusSucceed: {}, - } + errBranchNotFound = errors.New("branch not found") + errNoJobForBranch = errors.New("current branch has no jobs") + errNilBranch = errors.New("branch is nil") ) const ( @@ -50,6 +47,10 @@ const ( amplifyDefaultDomain = "amplifyapp.com" ) +// AmplifyPreview is used to get/create amplify preview +// deployments on AWS Amplify across multiple apps +// to work around hard limit 50 branches per app +// https://docs.aws.amazon.com/amplify/latest/userguide/quotas-chapter.html type AmplifyPreview struct { appIDs []string branchName string @@ -97,7 +98,7 @@ func (amp *AmplifyPreview) FindExistingBranch(ctx context.Context) (*types.Branc for resp := range resultCh { var errNotFound *types.NotFoundException if errors.As(resp.err, &errNotFound) { - logger.Debug("Branch not found", logKeyAppID, resp.appID, logKeyBranchName, amp.branchName) + slog.Debug("Branch not found", logKeyAppID, resp.appID, logKeyBranchName, amp.branchName) continue } else if resp.err != nil { failedResp.perAppErr[resp.appID] = resp.err @@ -126,20 +127,20 @@ func (amp *AmplifyPreview) CreateBranch(ctx context.Context) (*types.Branch, err resp, err := amp.client.CreateBranch(ctx, &lify.CreateBranchInput{ AppId: aws.String(appID), BranchName: aws.String(amp.branchName), - Description: aws.String("Branch generated for PR TODO"), + Description: aws.String("Branch created from 'amplify-preview' GHA action"), Stage: types.StagePullRequest, EnableAutoBuild: aws.Bool(true), }) var errLimitExceeded *types.LimitExceededException if errors.As(err, &errLimitExceeded) { - logger.Debug("Reached branches limit", logKeyAppID, appID) + slog.Debug("Reached branches limit", logKeyAppID, appID) } else if err != nil { failedResp.perAppErr[appID] = err } if resp != nil { - logger.Info("Successfully created branch", logKeyAppID, appID, logKeyBranchName, *resp.Branch.BranchName, logKeyJobID, resp.Branch.ActiveJobId) + slog.Info("Successfully created branch", logKeyAppID, appID, logKeyBranchName, *resp.Branch.BranchName, logKeyJobID, resp.Branch.ActiveJobId) return resp.Branch, nil } } @@ -148,6 +149,9 @@ func (amp *AmplifyPreview) CreateBranch(ctx context.Context) (*types.Branch, err } func (amp *AmplifyPreview) StartJob(ctx context.Context, branch *types.Branch) (*types.JobSummary, error) { + if branch == nil { + return nil, errNilBranch + } appID, err := appIDFromBranchARN(*branch.BranchArn) if err != nil { return nil, err @@ -164,13 +168,16 @@ func (amp *AmplifyPreview) StartJob(ctx context.Context, branch *types.Branch) ( return nil, err } - logger.Info("Successfully started job", logKeyAppID, appID, logKeyBranchName, *branch.BranchName, logKeyJobID, *resp.JobSummary.JobId) + slog.Info("Successfully started job", logKeyAppID, appID, logKeyBranchName, *branch.BranchName, logKeyJobID, *resp.JobSummary.JobId) return resp.JobSummary, nil } func (amp *AmplifyPreview) findJobsByID(ctx context.Context, branch *types.Branch, includeLatest bool, ids ...string) (jobSummaries []types.JobSummary, err error) { + if branch == nil { + return nil, errNilBranch + } appID, err := appIDFromBranchARN(*branch.BranchArn) if err != nil { return nil, err @@ -193,9 +200,15 @@ func (amp *AmplifyPreview) findJobsByID(ctx context.Context, branch *types.Branc } for _, id := range ids { - wantJobID, _ := strconv.Atoi(id) + wantJobID, err := strconv.Atoi(id) + if err != nil { + slog.Debug("unexpected Job ID value", logKeyJobID, wantJobID) + } for _, j := range resp.JobSummaries { - jobID, _ := strconv.Atoi(*j.JobId) + jobID, err := strconv.Atoi(*j.JobId) + if err != nil { + slog.Debug("unexpected Job ID value", logKeyJobID, jobID) + } if jobID == wantJobID { jobSummaries = append(jobSummaries, j) break @@ -208,6 +221,10 @@ func (amp *AmplifyPreview) findJobsByID(ctx context.Context, branch *types.Branc } func (amp *AmplifyPreview) GetLatestAndActiveJobs(ctx context.Context, branch *types.Branch) (latestJob, activeJob *types.JobSummary, err error) { + if branch == nil { + return nil, nil, errNilBranch + } + var jobIDs []string if branch.ActiveJobId != nil { jobIDs = append(jobIDs, *branch.ActiveJobId) @@ -228,7 +245,7 @@ func (amp *AmplifyPreview) GetLatestAndActiveJobs(ctx context.Context, branch *t } func (amp *AmplifyPreview) WaitForJobCompletion(ctx context.Context, branch *types.Branch, job *types.JobSummary) (currentJob, activeJob *types.JobSummary, err error) { - for i := 0; i < jobWaitTimeAttempts; i++ { + for i := range jobWaitTimeAttempts { jobSummaries, err := amp.findJobsByID(ctx, branch, true, *job.JobId) if err != nil { return nil, nil, err @@ -243,7 +260,7 @@ func (amp *AmplifyPreview) WaitForJobCompletion(ctx context.Context, branch *typ break } - logger.Info("Job is not in a completed state yet. Sleeping...", + slog.Info(fmt.Sprintf("Job is not in a completed state yet. Sleeping for %s", jobWaitSleepTime.String()), logKeyBranchName, amp.branchName, "job_status", currentJob.Status, "job_id", *currentJob.JobId, "attempts_left", jobWaitTimeAttempts-i) time.Sleep(jobWaitSleepTime) } @@ -265,8 +282,12 @@ func appIDFromBranchARN(branchArn string) (string, error) { } func isAmplifyJobCompleted(job *types.JobSummary) bool { - _, ok := amplifyJobCompletedStatuses[job.Status] - return ok + switch job.Status { + case types.JobStatusFailed, types.JobStatusCancelled, types.JobStatusSucceed: + return true + default: + return false + } } func (err aggregatedError) Error() error { diff --git a/tools/amplify-preview/github.go b/tools/amplify-preview/github.go index 203fc3d..578f3a9 100644 --- a/tools/amplify-preview/github.go +++ b/tools/amplify-preview/github.go @@ -20,7 +20,6 @@ import ( "context" "errors" "fmt" - "log" "os" "strconv" "strings" @@ -28,15 +27,11 @@ import ( "github.com/gravitational/shared-workflows/libs/github" ) -const ( - prRefNameSuffix = "/merge" -) - func postPreviewURL(ctx context.Context, commentBody string) error { + const prRefNameSuffix = "/merge" refName := os.Getenv("GITHUB_REF_NAME") githubRepository := os.Getenv("GITHUB_REPOSITORY") if !strings.HasSuffix(refName, prRefNameSuffix) { - return nil } gh, err := github.NewClientFromGHAuth(ctx) @@ -46,17 +41,21 @@ func postPreviewURL(ctx context.Context, commentBody string) error { prID, err := strconv.Atoi(strings.TrimSuffix(refName, "/merge")) if err != nil { - log.Fatalf("Failed to extract PR ID from GITHUB_REF_NAME=%s: %s", refName, err) + return fmt.Errorf("Failed to extract PR ID from GITHUB_REF_NAME=%s: %s", refName, err) } targetComment := github.CommentTraits{ BodyContains: amplifyMarkdownHeader, } + githubRepoParts := strings.Split(githubRepository, "/") + if len(githubRepoParts) < 2 { + return fmt.Errorf("Couldn't extract repo and owner from %q", githubRepository) + } currentPR := github.IssueIdentifier{ Number: prID, - Owner: strings.Split(githubRepository, "/")[0], - Repo: strings.Split(githubRepository, "/")[1], + Owner: githubRepoParts[0], + Repo: githubRepoParts[1], } comment, err := gh.FindCommentByTraits(ctx, currentPR, targetComment) diff --git a/tools/amplify-preview/main.go b/tools/amplify-preview/main.go index a0d3866..d9097a6 100644 --- a/tools/amplify-preview/main.go +++ b/tools/amplify-preview/main.go @@ -22,6 +22,7 @@ import ( "fmt" "log/slog" "os" + "os/signal" "strings" "time" @@ -34,8 +35,6 @@ import ( ) var ( - logger = slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelDebug})) - amplifyAppIDs = kingpin.Flag("amplify-app-ids", "List of Amplify App IDs").Envar("AMPLIFY_APP_IDS").Required().Strings() gitBranchName = kingpin.Flag("git-branch-name", "Git branch name").Envar("GIT_BRANCH_NAME").Required().String() createBranches = kingpin.Flag("create-branches", @@ -51,19 +50,26 @@ const ( func main() { kingpin.Parse() - ctx := context.Background() + slog.SetDefault(slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelDebug}))) + ctx, cancel := handleInterruption(context.Background()) + defer cancel() + + if err := run(ctx); err != nil { + slog.Error("run failed", "error", err) + os.Exit(1) + } +} +func run(ctx context.Context) error { cfg, err := config.LoadDefaultConfig(ctx, config.WithRetryer(func() aws.Retryer { return retry.AddWithMaxAttempts(retry.NewStandard(), 10) })) if err != nil { - logger.Error("failed to load configuration", "error", err) - os.Exit(1) + return fmt.Errorf("failed to load AWS configuration: %w", err) } if amplifyAppIDs == nil || len(*amplifyAppIDs) == 0 { - logger.Error("expected one more more Amplify App IDs") - os.Exit(1) + return errors.New("expected one more more Amplify App IDs") } amp := AmplifyPreview{ @@ -78,13 +84,6 @@ func main() { amp.appIDs = strings.Split(amp.appIDs[0], ",") } - if err := execute(ctx, amp); err != nil { - logger.Error("execution failed", "error", err) - os.Exit(1) - } -} - -func execute(ctx context.Context, amp AmplifyPreview) error { branch, err := ensureAmplifyBranch(ctx, amp) if err != nil { return err @@ -99,7 +98,7 @@ func execute(ctx context.Context, amp AmplifyPreview) error { return fmt.Errorf("failed to post preview URL: %w", err) } - logger.Info("Successfully posted PR comment") + slog.Info("Successfully posted PR comment") if *wait { currentJob, activeJob, err = amp.WaitForJobCompletion(ctx, branch, currentJob) @@ -114,7 +113,7 @@ func execute(ctx context.Context, amp AmplifyPreview) error { } if currentJob.Status == types.JobStatusFailed { - logger.Error("amplify job is in failed state", logKeyBranchName, amp.branchName, "job_status", currentJob.Status, "job_id", *currentJob.JobId) + slog.Error("amplify job is in failed state", logKeyBranchName, amp.branchName, "job_status", currentJob.Status, "job_id", *currentJob.JobId) return fmt.Errorf("amplify job is in %q state", currentJob.Status) } @@ -148,3 +147,22 @@ func ensureAmplifyDeployment(ctx context.Context, amp AmplifyPreview, branch *ty return nil, nil, fmt.Errorf("failed to lookup amplify job for branch %q: %w", amp.branchName, err) } } + +func handleInterruption(ctx context.Context) (context.Context, context.CancelFunc) { + ctx, cancel := context.WithCancel(ctx) + c := make(chan os.Signal, 1) + signal.Notify(c, os.Interrupt) + + go func() { + select { + case <-c: + cancel() + case <-ctx.Done(): + } + }() + + return ctx, func() { + signal.Stop(c) + cancel() + } +}