Skip to content

Commit d70e61b

Browse files
tphoneyactions-user
authored andcommitted
fix, cli getChangeUuid add retry and improve lookup (#2575)
Failed 3 attempts NB DEBUG is on, hence we see the re-try attempts <img width="1700" height="228" alt="image" src="https://github.com/user-attachments/assets/85215031-3424-446c-a35c-231515a21b38" /> Succeeds after 1 failure <img width="1715" height="394" alt="image" src="https://github.com/user-attachments/assets/c617e5b8-5155-46ef-967e-92ceedbf5611" /> GitOrigin-RevId: 3fb1af3de3311da4d8b993db16460f29d85fb70d
1 parent 7908ce0 commit d70e61b

File tree

10 files changed

+351
-229
lines changed

10 files changed

+351
-229
lines changed

cmd/changes_end_change.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ func EndChange(cmd *cobra.Command, args []string) error {
2626
return err
2727
}
2828

29-
changeUuid, err := getChangeUuid(ctx, oi, sdp.ChangeStatus_CHANGE_STATUS_HAPPENING, viper.GetString("ticket-link"), true)
29+
changeUuid, err := getChangeUUIDAndCheckStatus(ctx, oi, sdp.ChangeStatus_CHANGE_STATUS_HAPPENING, viper.GetString("ticket-link"), true)
3030
if err != nil {
3131
return loggedError{
3232
err: err,

cmd/changes_get_change.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ func GetChange(cmd *cobra.Command, args []string) error {
5252
return err
5353
}
5454

55-
changeUuid, err := getChangeUuid(ctx, oi, sdp.ChangeStatus(sdp.ChangeStatus_value[viper.GetString("status")]), viper.GetString("ticket-link"), true)
55+
changeUuid, err := getChangeUUIDAndCheckStatus(ctx, oi, sdp.ChangeStatus(sdp.ChangeStatus_value[viper.GetString("status")]), viper.GetString("ticket-link"), true)
5656
if err != nil {
5757
return loggedError{
5858
err: err,

cmd/changes_start_change.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ func StartChange(cmd *cobra.Command, args []string) error {
2626
return err
2727
}
2828

29-
changeUuid, err := getChangeUuid(ctx, oi, sdp.ChangeStatus_CHANGE_STATUS_DEFINING, viper.GetString("ticket-link"), true)
29+
changeUuid, err := getChangeUUIDAndCheckStatus(ctx, oi, sdp.ChangeStatus_CHANGE_STATUS_DEFINING, viper.GetString("ticket-link"), true)
3030
if err != nil {
3131
return loggedError{
3232
err: err,

cmd/changes_submit_plan.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ func SubmitPlan(cmd *cobra.Command, args []string) error {
135135
delete(lf, "file")
136136

137137
client := AuthenticatedChangesClient(ctx, oi)
138-
changeUuid, err := getChangeUuid(ctx, oi, sdp.ChangeStatus_CHANGE_STATUS_DEFINING, viper.GetString("ticket-link"), false)
138+
changeUuid, err := getChangeUUIDAndCheckStatus(ctx, oi, sdp.ChangeStatus_CHANGE_STATUS_DEFINING, viper.GetString("ticket-link"), false)
139139
if err != nil {
140140
return loggedError{
141141
err: err,

cmd/changes_submit_signal.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ func SubmitSignal(cmd *cobra.Command, args []string) error {
3838
if viper.GetString("description") == "" {
3939
return flagError{"--description is required"}
4040
}
41-
changeUUID, err := getChangeUuid(ctx, oi, sdp.ChangeStatus_CHANGE_STATUS_DEFINING, viper.GetString("ticket-link"), true)
41+
changeUUID, err := getChangeUUIDAndCheckStatus(ctx, oi, sdp.ChangeStatus_CHANGE_STATUS_DEFINING, viper.GetString("ticket-link"), true)
4242
if err != nil {
4343
return loggedError{
4444
err: err,

cmd/root.go

Lines changed: 66 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -183,9 +183,9 @@ Then enter the code:
183183
%v
184184
`
185185

186-
// getChangeUuid returns the UUID of a change, as selected by --uuid or --change, or a change with the specified status and having --ticket-link
187-
func getChangeUuid(ctx context.Context, oi sdp.OvermindInstance, expectedStatus sdp.ChangeStatus, ticketLink string, errNotFound bool) (uuid.UUID, error) {
188-
var changeUuid uuid.UUID
186+
// getChangeUUIDAndCheckStatus returns the UUID of a change, as selected by --uuid or --change, or a change with the specified status and having --ticket-link
187+
func getChangeUUIDAndCheckStatus(ctx context.Context, oi sdp.OvermindInstance, expectedStatus sdp.ChangeStatus, ticketLink string, errorOnNotFound bool) (uuid.UUID, error) {
188+
var changeUUID uuid.UUID
189189
var err error
190190

191191
uuidString := viper.GetString("uuid")
@@ -198,47 +198,86 @@ func getChangeUuid(ctx context.Context, oi sdp.OvermindInstance, expectedStatus
198198

199199
// Check UUID first if more than one is set
200200
if uuidString != "" {
201-
changeUuid, err = uuid.Parse(uuidString)
201+
changeUUID, err = uuid.Parse(uuidString)
202202
if err != nil {
203203
return uuid.Nil, fmt.Errorf("invalid --uuid value '%v', error: %w", uuidString, err)
204204
}
205205

206-
return changeUuid, nil
206+
return changeUUID, nil
207207
}
208208

209209
// Then check for a change URL
210210
if changeUrlString != "" {
211211
return parseChangeUrl(changeUrlString)
212212
}
213213

214-
// Finally look through all open changes to find one with a matching ticket link
215-
client := AuthenticatedChangesClient(ctx, oi)
216-
217-
changesList, err := client.ListChangesByStatus(ctx, &connect.Request[sdp.ListChangesByStatusRequest]{
218-
Msg: &sdp.ListChangesByStatusRequest{
219-
Status: expectedStatus,
220-
},
221-
})
222-
if err != nil {
223-
return uuid.Nil, fmt.Errorf("failed to search for existing changes: %w", err)
214+
// Finally look up by ticket link with retry
215+
changeUUID, err = getChangeByTicketLinkWithRetry(ctx, oi, ticketLink, expectedStatus, errorOnNotFound)
216+
if errorOnNotFound && err != nil {
217+
return uuid.Nil, err
224218
}
225219

226-
var maybeChangeUuid *uuid.UUID
227-
for _, c := range changesList.Msg.GetChanges() {
228-
if c.GetProperties().GetTicketLink() == ticketLink {
229-
maybeChangeUuid = c.GetMetadata().GetUUIDParsed()
230-
if maybeChangeUuid != nil {
231-
changeUuid = *maybeChangeUuid
232-
break
220+
return changeUUID, nil
221+
}
222+
223+
// getChangeByTicketLinkWithRetry performs the GetChangeByTicketLink API call with retry logic,
224+
// retrying both on error and when the status does not match the expected status.
225+
// NB api-server will only return the latest change with this ticket link.
226+
func getChangeByTicketLinkWithRetry(ctx context.Context, oi sdp.OvermindInstance, ticketLink string, expectedStatus sdp.ChangeStatus, errorOnNotFound bool) (uuid.UUID, error) {
227+
client := AuthenticatedChangesClient(ctx, oi)
228+
229+
var change *connect.Response[sdp.GetChangeResponse]
230+
var currentStatus sdp.ChangeStatus
231+
var err error
232+
maxRetries := 3
233+
if !errorOnNotFound {
234+
// If not erroring on not found, only attempt once.
235+
maxRetries = 1
236+
}
237+
retryDelay := 3 * time.Second
238+
239+
for attempt := 1; attempt <= maxRetries; attempt++ {
240+
change, err = client.GetChangeByTicketLink(ctx, &connect.Request[sdp.GetChangeByTicketLinkRequest]{
241+
Msg: &sdp.GetChangeByTicketLinkRequest{
242+
TicketLink: ticketLink,
243+
},
244+
})
245+
if err == nil {
246+
// change found
247+
var uuidPtr *uuid.UUID
248+
if change != nil && change.Msg != nil && change.Msg.GetChange() != nil && change.Msg.GetChange().GetMetadata() != nil {
249+
uuidPtr = change.Msg.GetChange().GetMetadata().GetUUIDParsed()
250+
currentStatus = change.Msg.GetChange().GetMetadata().GetStatus()
251+
if uuidPtr != nil && (currentStatus == expectedStatus) {
252+
// Success: we have a UUID and status matches the expected status
253+
return *uuidPtr, nil
254+
}
255+
}
256+
}
257+
// Log the error and retry if not the last attempt
258+
if attempt < maxRetries {
259+
logFields := log.Fields{
260+
"ovm.change.ticketLink": ticketLink,
261+
"expectedStatus": expectedStatus.String(),
262+
"attempt": attempt,
263+
"maxRetries": maxRetries,
264+
"currentStatus": currentStatus.String(),
265+
}
266+
if err != nil {
267+
logFields["error"] = err.Error()
268+
log.WithContext(ctx).WithFields(logFields).Debug("failed to get change by ticket link, retrying")
269+
} else {
270+
log.WithContext(ctx).WithFields(logFields).Debug("change found but status does not match, retrying")
233271
}
272+
time.Sleep(retryDelay)
234273
}
235274
}
236-
237-
if errNotFound && changeUuid == uuid.Nil {
238-
return uuid.Nil, fmt.Errorf("no change found with ticket link %v and status %v", ticketLink, expectedStatus.String())
275+
if err != nil {
276+
// Final attempt failed with an error
277+
return uuid.Nil, fmt.Errorf("error looking up change with ticket link %v after %d attempts: %w", ticketLink, maxRetries, err)
239278
}
240-
241-
return changeUuid, nil
279+
// Final attempt found a change but status did not match
280+
return uuid.Nil, fmt.Errorf("change %s found with ticket link %v. Change status %v does not match expected status %v after %d attempts", change.Msg.GetChange().GetMetadata().GetUUIDParsed(), ticketLink, currentStatus.String(), expectedStatus.String(), maxRetries)
242281
}
243282

244283
func parseChangeUrl(changeUrlString string) (uuid.UUID, error) {

cmd/terraform_apply.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ func TerraformApplyImpl(ctx context.Context, cmd *cobra.Command, oi sdp.Overmind
136136
}
137137
}
138138

139-
changeUuid, err := getChangeUuid(ctx, oi, sdp.ChangeStatus_CHANGE_STATUS_DEFINING, ticketLink, true)
139+
changeUuid, err := getChangeUUIDAndCheckStatus(ctx, oi, sdp.ChangeStatus_CHANGE_STATUS_DEFINING, ticketLink, true)
140140
if err != nil {
141141
return uuid.Nil, fmt.Errorf("failed to identify change: %w", err)
142142
}

cmd/terraform_plan.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ func TerraformPlanImpl(ctx context.Context, cmd *cobra.Command, oi sdp.OvermindI
218218
}
219219

220220
client := AuthenticatedChangesClient(ctx, oi)
221-
changeUuid, err := getChangeUuid(ctx, oi, sdp.ChangeStatus_CHANGE_STATUS_DEFINING, ticketLink, false)
221+
changeUuid, err := getChangeUUIDAndCheckStatus(ctx, oi, sdp.ChangeStatus_CHANGE_STATUS_DEFINING, ticketLink, false)
222222
if err != nil {
223223
uploadChangesSpinner.Fail(fmt.Sprintf("Uploading planned changes: failed searching for existing changes: %v", err))
224224
return nil

0 commit comments

Comments
 (0)