-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
dev: add flag for extra docker args #101931
dev: add flag for extra docker args #101931
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
For |
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.
My reasoning was that it seems the docker invocation could form part of multiple commands now and in the future, thus an environment variable would ensure being able to always supply additional arguments to the invocation. But that said, I'm not opposed to sticking to command-line arguments to keep things uniform.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @smg260 and @srosenberg)
79238b0
to
dc6aba8
Compare
4716ef9
to
0536a38
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.
@rickystewart Updated my PR to take command-line arguments instead.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @smg260 and @srosenberg)
0536a38
to
9852ed3
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 all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rickystewart and @smg260)
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.
LGTM. Consider bumping the version number in the top-level ./dev
script so people pick up the change.
In order to support git alternates an additional volume must be supplied to the bazel support docker. The TeamCity script already supplies a utility method called `run_bazel` which supplies the volume to support git alternates. This change allows for extra arguments to be passed via a command line argument to the docker invocation that is utilised for cross builds by the `dev` utility.
9852ed3
to
8eecff7
Compare
TFTR! |
Build succeeded: |
In order to support git alternates an additional volume must be supplied to the
bazel support docker. The TeamCity script already supplies a utility method
called
run_bazel
which supplies the volume to support git alternates.This change allows for extra arguments to be passed via a command line argument
to the docker invocation that is utilised for cross builds by the
dev
utility.Resolves: #101829