-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
RFC replace copy_py alias by a function #21948
base: master
Are you sure you want to change the base?
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
@@ -31,14 +31,18 @@ if [[ -z ${DAGSTER_UI_DONT_BUILD_JS_BUNDLE+x} ]]; then | |||
fi | |||
|
|||
echo -e "--- \033[32m:truck: Copying files...\033[0m" | |||
alias copy_py="rsync -av \ |
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.
shopt -s expand_alias would need to go as well.
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.
would replace other instances as well if any, just fishing for alignment here.
what environments does it not work in? can you re-submit with "NO_SKIP" in the commit message so that buildkite actually runs the test that uses this to make sure this works there? I think i copied this pattern from elsewhere, i definitely did not invent this macro (i think it is used elsewhere) |
macosx ohmyzsh mostly as proposed in the eng onboarding doc - a few plugins and not much else i don't think I have anything fancy going on and after trying 2-3 things, I just decided to replace the alias. it's not the most portable approach vs a bash function. My rationale in not putting any effort to fix my environment is that this is an OSS project and we shouldn't expect all OSS to normalize their environment for something that is easily made cross compatible. |
in some environment, the script will not work as the alias copy_to is not recognize.
0b54df0
to
5f6c26b
Compare
TL;DR
The copy_py alias is not recognized in some environments, and the script is unusable as it fails to rsync the desired files.
This change replaces it with a more readily supported bash function