-
Notifications
You must be signed in to change notification settings - Fork 109
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 subtraction bug in start_turbo_cache.sh #407
Conversation
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.
Thanks a lot for finding this ❤️
I like these changes a lot, but they overlap with ongoing work to integrate shellcheck in a regression-safe manner via pre-commit hooks.
I'd like to suggest only making the - -
-> -
change in this commit and remove all the shellcheck fixes.
Then we can fix all shellcheck issues in a single commit, similar to #405 by adding this to the global tools/pre-commit-hooks.nix
file:
shellcheck.enable = true;
This way the nix flake check
that runs in CI will ensure that we don't regress shellcheck issues. @cormacrelf WDYT? Would you be interested in tackling this?
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @cormacrelf)
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.
FYI: We have been doing a lot of work behind the scenes to support Azure and GCP. We found some issues in the AWS example in the repo while supporting the other providers but haven't upstreamed them yet.
Thank you very much for your contributions ;-)
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @cormacrelf)
deployment-examples/terraform/AWS/scripts/start_turbo_cache.sh
line 35 at r4 (raw file):
# These environmental variables are used inside the json file resolution. export NATIVE_LINK_AWS_REGION="$AWS_REGION" NATIVE_LINK_AWS_S3_CAS_BUCKET=$(echo "$TAGS"| jq -r '.Tags[] | select(.Key == "native_link:s3_cas_bucket").Value')
nit: Any reason why you prefer define then export rather than doing both in one line?
deployment-examples/terraform/AWS/scripts/start_turbo_cache.sh
line 63 at r4 (raw file):
/usr/local/bin/native-link /root/scheduler.json elif [ "$TYPE" == "worker" ]; then SCHEDULER_URL=$(echo "$TAGS"| jq -r '.Tags[] | select(.Key == "native_link:scheduler_url").Value')
good catch :-)
…ployment If TOTAL_AVAIL_MEMORY was 100, then this computed $(( 100 - - 10 - 5 )) Which is actually 105. Now it's $(( 100 - 10 - 5 )) Which is 85, as the comment originally claimed.
It seems one of you wants me to remove the shellcheck fixes, the other wants me to do them slightly differently. I'm going to get rid of them. You'll see them again when you get CI to check it for you. I'm not using the AWS deployment at this stage. I might file an issue for the thing I'm thinking of doing (Nix shenanigans). |
9e1cf34
to
e82665c
Compare
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.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @cormacrelf)
I see now, I was confused because some things where shellchecked and some were not. I'm in support of adding shellcheck, however I personally go for readability/simplicity, so I'd prefer to never allow our code base to use |
Fair. The reason for the rule is that the export builtin always returns 0, so a $() command can fail silently. So it does not warn about I recommend shellcheck in the editor, e.g. via null-ls for Neovim. |
…ployment (TraceMachina#407) If TOTAL_AVAIL_MEMORY was 100, then this computed $(( 100 - - 10 - 5 )) Which is actually 105. Now it's $(( 100 - 10 - 5 )) Which is 85, as the comment originally claimed.
Description
I saw this while browsing through the code. There was a double negative sign.
While I was there, I accepted some shellcheck recommendations, which you can
see in the individual commits.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
It has not been tested.
Checklist
reviewability. You are welcome to squash it at merge time.)
This change is