Skip to content

Commit 0d00076

Browse files
committed
Address PR feedback
1 parent 80abb12 commit 0d00076

File tree

3 files changed

+147
-82
lines changed

3 files changed

+147
-82
lines changed

libs/github/comment.go

+12-11
Original file line numberDiff line numberDiff line change
@@ -12,18 +12,21 @@ var (
1212
ErrCommentNotFound = errors.New("comment not found")
1313
)
1414

15+
// IssueIdentifier represents an issue or PR on GitHub
1516
type IssueIdentifier struct {
1617
Owner string
1718
Repo string
1819
Number int
1920
}
2021

21-
// every trait is treated as AND
22+
// CommentTraits defines optional traits to filter comments.
23+
// Every trait (if non-empty-string) is matched with an "AND" operator
2224
type CommentTraits struct {
23-
BodyContains *string
24-
UserLogin *string
25+
BodyContains string
26+
UserLogin string
2527
}
2628

29+
// FindCommentByTraits searches for a comment in an PR or issue based on specified traits
2730
func (c *Client) FindCommentByTraits(ctx context.Context, issue IssueIdentifier, targetComment CommentTraits) (*github.IssueComment, error) {
2831
comments, _, err := c.client.Issues.ListComments(ctx, issue.Owner, issue.Repo, issue.Number, nil)
2932
if err != nil {
@@ -32,16 +35,12 @@ func (c *Client) FindCommentByTraits(ctx context.Context, issue IssueIdentifier,
3235

3336
for _, c := range comments {
3437
matcher := true
35-
if targetComment.UserLogin != nil {
36-
matcher = matcher &&
37-
c.User != nil && c.User.Login != nil &&
38-
*c.User.Login == *targetComment.UserLogin
38+
if targetComment.UserLogin != "" {
39+
matcher = matcher && c.User.GetLogin() == targetComment.UserLogin
3940
}
4041

41-
if targetComment.BodyContains != nil {
42-
matcher = matcher &&
43-
c.Body != nil &&
44-
strings.Contains(*c.Body, *targetComment.BodyContains)
42+
if targetComment.BodyContains != "" {
43+
matcher = matcher && strings.Contains(c.GetBody(), targetComment.BodyContains)
4544
}
4645

4746
if matcher {
@@ -52,6 +51,7 @@ func (c *Client) FindCommentByTraits(ctx context.Context, issue IssueIdentifier,
5251
return nil, ErrCommentNotFound
5352
}
5453

54+
// CreateComment creates a new comment on an issue or PR
5555
func (c *Client) CreateComment(ctx context.Context, issue IssueIdentifier, commentBody string) error {
5656
_, _, err := c.client.Issues.CreateComment(ctx, issue.Owner, issue.Repo, issue.Number, &github.IssueComment{
5757
Body: &commentBody,
@@ -60,6 +60,7 @@ func (c *Client) CreateComment(ctx context.Context, issue IssueIdentifier, comme
6060
return err
6161
}
6262

63+
// UpdateComment updates an existing comment on an issue or PR
6364
func (c *Client) UpdateComment(ctx context.Context, issue IssueIdentifier, commentId int64, commentBody string) error {
6465
_, _, err := c.client.Issues.EditComment(ctx, issue.Owner, issue.Repo, commentId, &github.IssueComment{
6566
Body: &commentBody,

tools/amplify-preview/amplify.go

+67-17
Original file line numberDiff line numberDiff line change
@@ -51,16 +51,17 @@ const (
5151
)
5252

5353
type AmplifyPreview struct {
54-
appIDs []string
55-
client *amplify.Client
54+
appIDs []string
55+
branchName string
56+
client *amplify.Client
5657
}
5758

5859
type aggregatedError struct {
5960
perAppErr map[string]error
6061
message string
6162
}
6263

63-
func (amp *AmplifyPreview) FindExistingBranch(ctx context.Context, branchName string) (*types.Branch, error) {
64+
func (amp *AmplifyPreview) FindExistingBranch(ctx context.Context) (*types.Branch, error) {
6465
type resp struct {
6566
appID string
6667
data *amplify.GetBranchOutput
@@ -75,7 +76,7 @@ func (amp *AmplifyPreview) FindExistingBranch(ctx context.Context, branchName st
7576
defer wg.Done()
7677
branch, err := amp.client.GetBranch(ctx, &amplify.GetBranchInput{
7778
AppId: aws.String(appID),
78-
BranchName: aws.String(branchName),
79+
BranchName: aws.String(amp.branchName),
7980
})
8081
resultCh <- resp{
8182
appID: appID,
@@ -96,7 +97,7 @@ func (amp *AmplifyPreview) FindExistingBranch(ctx context.Context, branchName st
9697
for resp := range resultCh {
9798
var errNotFound *types.NotFoundException
9899
if errors.As(resp.err, &errNotFound) {
99-
logger.Debug("Branch not found", logKeyAppID, resp.appID, logKeyBranchName, branchName)
100+
logger.Debug("Branch not found", logKeyAppID, resp.appID, logKeyBranchName, amp.branchName)
100101
continue
101102
} else if resp.err != nil {
102103
failedResp.perAppErr[resp.appID] = resp.err
@@ -115,7 +116,7 @@ func (amp *AmplifyPreview) FindExistingBranch(ctx context.Context, branchName st
115116
return nil, errBranchNotFound
116117
}
117118

118-
func (amp *AmplifyPreview) CreateBranch(ctx context.Context, branchName string) (*types.Branch, error) {
119+
func (amp *AmplifyPreview) CreateBranch(ctx context.Context) (*types.Branch, error) {
119120
failedResp := aggregatedError{
120121
perAppErr: map[string]error{},
121122
message: "failed to create branch",
@@ -124,7 +125,7 @@ func (amp *AmplifyPreview) CreateBranch(ctx context.Context, branchName string)
124125
for _, appID := range amp.appIDs {
125126
resp, err := amp.client.CreateBranch(ctx, &amplify.CreateBranchInput{
126127
AppId: aws.String(appID),
127-
BranchName: aws.String(branchName),
128+
BranchName: aws.String(amp.branchName),
128129
Description: aws.String("Branch generated for PR TODO"),
129130
Stage: types.StagePullRequest,
130131
EnableAutoBuild: aws.Bool(true),
@@ -169,38 +170,87 @@ func (amp *AmplifyPreview) StartJob(ctx context.Context, branch *types.Branch) (
169170

170171
}
171172

172-
func (amp *AmplifyPreview) GetLatestAndActiveJobs(ctx context.Context, branch *types.Branch) (latestJob, activeJob *types.JobSummary, err error) {
173+
func (amp *AmplifyPreview) findJobsByID(ctx context.Context, branch *types.Branch, includeLatest bool, ids ...string) (jobSummaries []types.JobSummary, err error) {
173174
appID, err := appIDFromBranchARN(*branch.BranchArn)
174175
if err != nil {
175-
return nil, nil, err
176+
return nil, err
176177
}
177178

178179
resp, err := amp.client.ListJobs(ctx, &amplify.ListJobsInput{
179180
AppId: aws.String(appID),
180-
BranchName: branch.BranchName,
181+
BranchName: aws.String(amp.branchName),
181182
MaxResults: 50,
182183
})
183184
if err != nil {
184-
return nil, nil, err
185+
return nil, err
185186
}
186187
if len(resp.JobSummaries) == 0 {
187-
return nil, nil, errNoJobForBranch
188+
return nil, errNoJobForBranch
188189
}
189190

190-
latestJob = &resp.JobSummaries[0]
191-
if branch.ActiveJobId != nil {
191+
if includeLatest {
192+
jobSummaries = append(jobSummaries, resp.JobSummaries[0])
193+
}
194+
195+
for _, id := range ids {
196+
wantJobID, _ := strconv.Atoi(id)
192197
for _, j := range resp.JobSummaries {
193198
jobID, _ := strconv.Atoi(*j.JobId)
194-
activeJobID, _ := strconv.Atoi(*branch.ActiveJobId)
195-
if jobID == activeJobID {
196-
activeJob = &j
199+
if jobID == wantJobID {
200+
jobSummaries = append(jobSummaries, j)
201+
break
197202
}
198203
}
204+
205+
}
206+
207+
return jobSummaries, nil
208+
}
209+
210+
func (amp *AmplifyPreview) GetLatestAndActiveJobs(ctx context.Context, branch *types.Branch) (latestJob, activeJob *types.JobSummary, err error) {
211+
var jobIDs []string
212+
if branch.ActiveJobId != nil {
213+
jobIDs = append(jobIDs, *branch.ActiveJobId)
214+
}
215+
jobSummaries, err := amp.findJobsByID(ctx, branch, true, jobIDs...)
216+
if err != nil {
217+
return nil, nil, err
218+
}
219+
220+
if len(jobSummaries) > 0 {
221+
latestJob = &jobSummaries[0]
222+
}
223+
if len(jobSummaries) > 1 {
224+
activeJob = &jobSummaries[1]
199225
}
200226

201227
return latestJob, activeJob, nil
202228
}
203229

230+
func (amp *AmplifyPreview) WaitForJobCompletion(ctx context.Context, branch *types.Branch, job *types.JobSummary) (currentJob, activeJob *types.JobSummary, err error) {
231+
for i := 0; i < jobWaitTimeAttempts; i++ {
232+
jobSummaries, err := amp.findJobsByID(ctx, branch, true, *job.JobId)
233+
if err != nil {
234+
return nil, nil, err
235+
}
236+
if len(jobSummaries) > 0 {
237+
currentJob = &jobSummaries[0]
238+
}
239+
if len(jobSummaries) > 1 {
240+
activeJob = &jobSummaries[1]
241+
}
242+
if isAmplifyJobCompleted(currentJob) {
243+
break
244+
}
245+
246+
logger.Info("Job is not in a completed state yet. Sleeping...",
247+
logKeyBranchName, amp.branchName, "job_status", currentJob.Status, "job_id", *currentJob.JobId, "attempts_left", jobWaitTimeAttempts-i)
248+
time.Sleep(jobWaitSleepTime)
249+
}
250+
251+
return currentJob, activeJob, nil
252+
}
253+
204254
func appIDFromBranchARN(branchArn string) (string, error) {
205255
parsedArn, err := arn.Parse(branchArn)
206256
if err != nil {

tools/amplify-preview/main.go

+68-54
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package main
1919
import (
2020
"context"
2121
"errors"
22+
"fmt"
2223
"log/slog"
2324
"os"
2425
"strings"
@@ -52,85 +53,98 @@ func main() {
5253
kingpin.Parse()
5354
ctx := context.Background()
5455

55-
cfg, err := config.LoadDefaultConfig(context.TODO(), config.WithRetryer(func() aws.Retryer {
56+
cfg, err := config.LoadDefaultConfig(ctx, config.WithRetryer(func() aws.Retryer {
5657
return retry.AddWithMaxAttempts(retry.NewStandard(), 10)
5758
}))
5859
if err != nil {
5960
logger.Error("failed to load configuration", "error", err)
6061
os.Exit(1)
6162
}
6263

64+
if amplifyAppIDs == nil || len(*amplifyAppIDs) == 0 {
65+
logger.Error("expected one more more Amplify App IDs")
66+
os.Exit(1)
67+
}
68+
6369
amp := AmplifyPreview{
64-
client: amplify.NewFromConfig(cfg),
65-
appIDs: func() []string {
66-
if len(*amplifyAppIDs) == 1 {
67-
// kingpin env variables are separated by new lines, and there is no way to change the behavior
68-
// https://github.com/alecthomas/kingpin/issues/249
69-
return strings.Split((*amplifyAppIDs)[0], ",")
70-
}
71-
return *amplifyAppIDs
72-
}(),
70+
client: amplify.NewFromConfig(cfg),
71+
branchName: *gitBranchName,
72+
appIDs: *amplifyAppIDs,
7373
}
7474

75-
// Check if Amplify branch is already connected to one of the Amplify Apps
76-
branch, err := amp.FindExistingBranch(ctx, *gitBranchName)
77-
if err != nil {
78-
if !*createBranches && !errors.Is(err, errNoJobForBranch) {
79-
logger.Error("failed to lookup branch", logKeyBranchName, *gitBranchName, "error", err)
80-
os.Exit(1)
81-
}
75+
if len(amp.appIDs) == 1 {
76+
// kingpin env variables are separated by new lines, and there is no way to change the behavior
77+
// https://github.com/alecthomas/kingpin/issues/249
78+
amp.appIDs = strings.Split(amp.appIDs[0], ",")
79+
}
8280

83-
// If branch wasn't found, and branch creation enabled - create new branch
84-
branch, err = amp.CreateBranch(ctx, *gitBranchName)
85-
if err != nil {
86-
logger.Error("failed to create branch", logKeyBranchName, *gitBranchName, "error", err)
87-
os.Exit(1)
88-
}
81+
if err := execute(ctx, amp); err != nil {
82+
logger.Error("execution failed", "error", err)
83+
os.Exit(1)
8984
}
85+
}
9086

91-
// check if existing branch was/being deployed already
92-
latestJob, activeJob, err := amp.GetLatestAndActiveJobs(ctx, branch)
87+
func execute(ctx context.Context, amp AmplifyPreview) error {
88+
branch, err := ensureAmplifyBranch(ctx, amp)
9389
if err != nil {
94-
if !*createBranches && !errors.Is(err, errNoJobForBranch) {
95-
logger.Error("failed to get amplify job", logKeyBranchName, *gitBranchName, "error", err)
96-
os.Exit(1)
97-
}
90+
return err
91+
}
9892

99-
// if job not found and branch was just created - start new job
100-
latestJob, err = amp.StartJob(ctx, branch)
101-
if err != nil {
102-
logger.Error("failed to start amplify job", logKeyBranchName, *gitBranchName, "error", err)
103-
os.Exit(1)
104-
}
93+
currentJob, activeJob, err := ensureAmplifyDeployment(ctx, amp, branch)
94+
if err != nil {
95+
return err
10596
}
10697

107-
if err := postPreviewURL(ctx, amplifyJobsToMarkdown(branch, latestJob, activeJob)); err != nil {
108-
logger.Error("failed to post preview URL", "error", err)
109-
os.Exit(1)
98+
if err := postPreviewURL(ctx, amplifyJobsToMarkdown(branch, currentJob, activeJob)); err != nil {
99+
return fmt.Errorf("failed to post preview URL: %w", err)
110100
}
101+
111102
logger.Info("Successfully posted PR comment")
112103

113104
if *wait {
114-
for i := 0; !isAmplifyJobCompleted(latestJob) && i < jobWaitTimeAttempts; i++ {
115-
latestJob, activeJob, err = amp.GetLatestAndActiveJobs(ctx, branch)
116-
if err != nil {
117-
logger.Error("failed to get amplify job", logKeyBranchName, *gitBranchName, "error", err)
118-
os.Exit(1)
119-
}
120-
121-
logger.Info("Job is not in a completed state yet. Sleeping...",
122-
logKeyBranchName, *gitBranchName, "job_status", latestJob.Status, "job_id", *latestJob.JobId, "attempts_left", jobWaitTimeAttempts-i)
123-
time.Sleep(jobWaitSleepTime)
105+
currentJob, activeJob, err = amp.WaitForJobCompletion(ctx, branch, currentJob)
106+
if err != nil {
107+
return fmt.Errorf("failed to follow job status: %w", err)
124108
}
125109

126-
if err := postPreviewURL(ctx, amplifyJobsToMarkdown(branch, latestJob, activeJob)); err != nil {
127-
logger.Error("failed to post preview URL", "error", err)
128-
os.Exit(1)
110+
// update comment when job is completed
111+
if err := postPreviewURL(ctx, amplifyJobsToMarkdown(branch, currentJob, activeJob)); err != nil {
112+
return fmt.Errorf("failed to post preview URL: %w", err)
129113
}
130114
}
131115

132-
if latestJob.Status == types.JobStatusFailed {
133-
logger.Error("amplify job is in failed state", logKeyBranchName, *gitBranchName, "job_status", latestJob.Status, "job_id", *latestJob.JobId)
134-
os.Exit(1)
116+
if currentJob.Status == types.JobStatusFailed {
117+
logger.Error("amplify job is in failed state", logKeyBranchName, amp.branchName, "job_status", currentJob.Status, "job_id", *currentJob.JobId)
118+
return fmt.Errorf("amplify job is in %q state", currentJob.Status)
119+
}
120+
121+
return nil
122+
}
123+
124+
// ensureAmplifyBranch checks if git branch is connected to amplify across multiple amplify apps
125+
// if "create-branch" is enabled, then branch is created if not found, otherwise returns error
126+
func ensureAmplifyBranch(ctx context.Context, amp AmplifyPreview) (*types.Branch, error) {
127+
branch, err := amp.FindExistingBranch(ctx)
128+
if err == nil {
129+
return branch, nil
130+
} else if errors.Is(err, errBranchNotFound) && *createBranches {
131+
return amp.CreateBranch(ctx)
132+
} else {
133+
return nil, fmt.Errorf("failed to lookup branch %q: %w", amp.branchName, err)
134+
}
135+
}
136+
137+
// ensureAmplifyDeployment lists deployments and checks for latest and active (the one that's currently live) deployments
138+
// if this branch has no deployments yet and "create-branch" is enabled, then deployment will be kicked off
139+
// this is because when new branch is created on Amplify and "AutoBuild" is enabled, it won't start the deployment until next commit
140+
func ensureAmplifyDeployment(ctx context.Context, amp AmplifyPreview, branch *types.Branch) (currentJob, activeJob *types.JobSummary, err error) {
141+
currentJob, activeJob, err = amp.GetLatestAndActiveJobs(ctx, branch)
142+
if err == nil {
143+
return currentJob, activeJob, nil
144+
} else if errors.Is(err, errNoJobForBranch) && *createBranches {
145+
return nil, nil, fmt.Errorf("failed to lookup amplify job for branch %q: %w", amp.branchName, err)
146+
} else {
147+
currentJob, err = amp.StartJob(ctx, branch)
148+
return currentJob, activeJob, err
135149
}
136150
}

0 commit comments

Comments
 (0)