Skip to content
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

Fix GCP image bake #3428

Merged
merged 13 commits into from
Jul 13, 2022
Merged

Fix GCP image bake #3428

merged 13 commits into from
Jul 13, 2022

Conversation

dmanjunath
Copy link
Contributor

@dmanjunath dmanjunath commented Jul 12, 2022

Description

GCP Image bake has been broken for ~2 weeks. I think there's two bugs that this PR fixes

  1. cd $PROTOCOL_DIR inside the ssh terminal is not the correct value so it was throwing an error
  2. the trap_exit didn't return exit $?, added this in

Tests

Run the GCP bake process on CI. Here's a passing run https://app.circleci.com/pipelines/github/AudiusProject/audius-protocol/24925/workflows/74d5bf45-3d0c-4839-b1a1-f46e7798113d/jobs/297214

Monitoring - How will this change be monitored? Are there sufficient logs / alerts?

CI will indicate if run is passing/failing

@dmanjunath dmanjunath marked this pull request as ready for review July 12, 2022 16:26
@@ -89,11 +92,13 @@ jobs:
A up;
A run monitoring up;
A down;
cd "$PROTOCOL_DIR";
cd audius-protocol/;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this env var is defined in the CI context, it's not passed through the ssh session which is why this was causing problems

echo $?
exit $?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was the other problem, we need to exit with this status code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you're returning the $? of echo $? which should always be 0.

Copy link
Contributor

@joaquincasares joaquincasares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a few suggested changes via #3453 that resolve the below comments.

echo $?
exit $?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you're returning the $? of echo $? which should always be 0.

git checkout -f;
exit;
EOF

echo "got status code from ssh agent" $?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we're only echoing the status, but not stopping, nor failing, the build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch with the echo. i'm gonna remove all the echos

as for the status echo, i believe circle stops if the exit status of a command is not 0, i think it implicitly has a set -e. thats why there's some || true in the code

@joaquincasares
Copy link
Contributor

Additionally, the command's block of text would be more readable and provide syntax highlighting out-of-the-box if the command simply called a bake.sh file.

Copy link
Contributor

@joaquincasares joaquincasares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it stands, gcloud compute instances stop $GCLOUD_VM_NAME will not be run if we encounter a non-zero exit code.

If this is acceptable, the PR looks good to merge.

@dmanjunath
Copy link
Contributor Author

@joaquincasares i tested that the trap runs the cleanup regardless of status code #2979 (comment)

@dmanjunath dmanjunath merged commit d481fe1 into master Jul 13, 2022
@dmanjunath dmanjunath deleted the dm-revert-fail-on-gcp-img-bake-err branch July 13, 2022 21:15
@joaquincasares
Copy link
Contributor

@joaquincasares i tested that the trap runs the cleanup regardless of status code #2979 (comment)

@dmanjunath got it! You're correct, we don't need to stop the instance, only delete the instance within trap cleanup EXIT.

The PR looks perfect!

sliptype pushed a commit that referenced this pull request Sep 10, 2023
[2c08c63] [C-2653] Add shallow argument to audius-query hooks (#3450) Andrew Mendelsohn
[0e5d1b3] [PAY-1247] Cap height of chat textinput on mobile (#3447) Reed
[efaa425] [PAY-1243] Mobile chat header touchable feedback (#3448) Reed
[3980e00] [C-2647] Use entire cache to denormalize data (#3449) Andrew Mendelsohn
[1395822] [PAY-1150] Disable scrolling when chat reaction popup is active (#3445) Reed
[f4ad5f7] [C-2592] Fix edit playlist (#3443) Kyle Shanks
[875054a] Fix mobile native change comment C-2657 (#3444) nicoback2
[63b5ab4] Bump SDK version and change walletapi to auth (#3440) nicoback2
[1611465] DMs: Use unblock/block confirmation modals on profile (#3437) Marcus Pasell
[179dda6] [PAY-1142] DMs: Update chat tail (#3438) Marcus Pasell
[95f1843] [PAY-946] Handle signed out users across DMs (#3423) Michael Piazza
[682eebb] [C-2649] Refactor desktop collection header (#3441) Dylan Jeffers
[7310fae] [PAY-800][PAY-1249][PAY-1250] Display track and playlist tiles in web chats - Part 1 (#3377) Saliou Diallo
[689fbcb] [PAY-1274] DMs: Unsticky inbox unavailable message (#3435) Marcus Pasell
[0212e19] Fix unread count incorrect on mobile chats (#3430) Reed
[6743352] Update legacy playlist screens to use legacy inputs (#3436) Kyle Shanks
[fb25e74] [CON-678] v2 user profile edit (#3431) Theo Ilie
[abf827f] [PAY-1255] [PAY-1245] DMs: Add confirmation modals (#3424) Marcus Pasell
[536d3c6] Make mobile chat inbox unavailable message unsticky (#3434) Reed
[f11f266] [PAY-1273] Fix scrolling issues (#3433) Michael Piazza
[26ec389] [PAY-1258] Add toast when chat messages are copied (#3421) Randy Schott
[9ba1956] [PAY-1251] [PAY-1188] DMs: Add error message to failed message sends (#3428) Marcus Pasell
[14034ae] [C-2597][C-2651] Add audius-query usage docs (#3427) Andrew Mendelsohn
[16deebc] [C-2508] Update playback positions to be tracked per user id (#3374) Kyle Shanks
[2d6b278] [C-2644] Fix issue where new playlist drag-drop not persisting (#3422) Dylan Jeffers
[a9c2c5e] [PAY-1212] [PAY-1146] Reactions and message UI polish (#3414) Michael Piazza
[ab8bd6b] Type fix for hook options (#3426) Andrew Mendelsohn
[d167810] [PAY-1272] Make desktop DM text copyable (#3425) Michael Piazza
[7ba5973] [PAY-925] Adds unread message icons on web (#3418) Randy Schott
[645f0a5] [PAY-935] Fix ChatHeader issue (#3419) Michael Piazza
[cc1c6e8] Update mobile playlist create flows to conform with new designs (#3416) Kyle Shanks
[9701930] [PAY-1260] New chat tails on mobile (#3420) Reed
[dd2873e] [PAY-1241] Skeleton for mobile chat list screen (#3393) Reed
[a373e62] [PAY-1253] Mobile message resend button (#3413) Reed
[ddc5540] Fix type error track availability modal C-2632 (#3415) nicoback2
[19929b9] [C-2601 C-2627 C-2628 C-2638] Playlist library QA (#3411) Dylan Jeffers
[5a7f61a] [C-2468] Add desktop create flow (#3409) Dylan Jeffers
[b0c1583] Mark chat as read on ChatScreen entry (#3410) Reed
[6e120cd] [PAY-1222] Fix unread messages count (#3407) Michael Piazza
[95dd306] [C-2299] Fix NowPlayingDrawer navigation deduping bug (#3405) Sebastian Klingler
[01fcab4] [PAY-1195] Mobile chat screen unavailable states (#3402) Reed
[67b1788] [C-2598] Add types for audius-query hooks (#3406) Andrew Mendelsohn
[8bf784a] Increase space between chat reaction and copy message button (#3403) Reed
[2cba881] Fix upload collection C-2630 (#3404) nicoback2
[aee7653] [PAY-1242] Mobile chats copy message button (#3399) Reed
[01ec1fb] [PAY-1220] Fix chat receive bug (#3400) Michael Piazza
[0ad899c] Add useAuthenticatedCallback (#3392) Dylan Jeffers
[412ca92] Improve popup-menu a11y (#3394) Dylan Jeffers
[ecdfb7e] [PAY-1136][PAY-1129] Add infinite loading to albums on desktop / mobile web (#3371) Randy Schott
[35678eb] [PAY-1028] Fix mobile chat actions drawer touch feedback (#3388) Reed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants