-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
update schedule and add input checking for replay-verify on archive #15501
Conversation
⏱️ 9h 51m total CI duration on this PR
🚨 4 jobs on the last run were significantly faster/slower than expected
|
@@ -14,7 +14,7 @@ on: | |||
- ".github/workflows/provision-replay-verify-archive-disks.yaml" | |||
- ".github/workflows/workflow-run-replay-verify-archive-storage-provision.yaml" | |||
schedule: | |||
- cron: "0 22 * * 1,3,5" # This runs every Mon,Wed,Fri |
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.
is this UTC? (You said you switch to night)
dec6780
to
6d909ec
Compare
b93f8c7
to
d1337b0
Compare
# This is in case user manually cancel the step above, we still want to cleanup the resources | ||
- name: Post-run cleanup | ||
env: | ||
GOOGLE_CLOUD_PROJECT: aptos-devinfra-0 | ||
if: ${{ always() }} | ||
run: | | ||
cd testsuite/replay-verify | ||
poetry run python main.py --network ${{ inputs.NETWORK }} --cleanup | ||
CMD="poetry run python main.py --network ${{ inputs.NETWORK }}" --cleanup |
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.
There appears to be a syntax error in the command string construction. The --cleanup
flag is placed outside the quotes, which will cause it to be interpreted as a separate shell command. The corrected version should be:
CMD="poetry run python main.py --network ${{ inputs.NETWORK }} --cleanup"
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
echo $DISK_URIS | ||
|
||
if [ -n "$DISK_URIS" ]; then | ||
gcloud compute disks delete $DISK_URIS |
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 $DISK_URIS
variable needs to be quoted in the gcloud
command to properly handle disk names containing spaces. The command should be:
gcloud compute disks delete "$DISK_URIS"
This ensures all disk URIs are passed correctly as a single argument to the delete command.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
d1337b0
to
0b11e19
Compare
DISK_URIS=$(gcloud compute disks list --filter="-users:* AND status=READY" --format "value(uri())") | ||
echo "Disks to be deleted:" | ||
echo "$DISK_URIS" | ||
|
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 gcloud compute disks delete
command expects each disk URI as a separate argument. The current syntax will pass all URIs as a single argument, causing the command to fail. Consider updating to:
echo "$DISK_URIS" | xargs -r gcloud compute disks delete
The -r
flag ensures xargs
only runs if there are disk URIs to process.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
2897d0d
to
b52b890
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
b52b890
to
0d42bb9
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
Description
How Has This Been Tested?
local tested
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist