Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Harden deploy migration flow during local and Azure deploys #148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Harden deploy migration flow during local and Azure deploys #148
Changes from all commits
644a8e80735ac2f289fcdbbbc08763a85e0edb7da94b4825f4cbb5e21efb3a3359b7491789cd4ac232118fbd55776de882fc44a028a1778545aa06829e006592224beFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waitForMigrationkeepslastStatusfrom the last successful HTTP response; if a later attempt errors (timeout/DNS/etc.), the progress output can still show a stalestatus=...even though the current attempt didn’t get a response. Consider resettinglastStatusto 0 on request errors (or tracking/printing the last error separately) so the status suffix always reflects an actual response.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
triggerAndWaitForMigrationWithClienttakes bothbaseURLand adevlakeClientthat already carries aBaseURL. If these ever diverge, migration trigger and migration wait will hit different instances. Consider deriving the wait URL fromdevlakeClient.BaseURL(or validating they match) to avoid accidental mismatches.This issue also appears on line 256 of the same file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In triggerAndWaitForMigrationWithClient, if an early trigger attempt fails and a later attempt succeeds, lastErr is never cleared. This leads to misleading output ("Continuing to monitor… anyway") and can produce an incorrect combined error claiming the trigger failed even when it eventually succeeded. Consider resetting lastErr to nil on success or tracking success with a separate boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When both the trigger and wait phases fail, the returned error wraps only the wait error (
%w) and interpolates the trigger error with%v, which loses the trigger error in the error chain. If callers/tests need to inspect both failures, consider returning a combined error (e.g., joining the two) while still preserving wrapping for each underlying error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migration helper tests use extremely small timeouts/intervals (1–5ms). These can be flaky on slower/loaded CI environments due to scheduler and timer granularity. Consider increasing the durations (while keeping the tests fast) to reduce nondeterminism, e.g., using tens of milliseconds and slightly larger retry intervals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new migration helper has an edge case where an early trigger failure followed by a later success should not be treated as a trigger failure (and should not produce the combined "trigger failed earlier" error). Adding a focused test for "first trigger fails, later succeeds, then wait fails" would lock this behavior in and prevent regressions.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.