-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
828: clean up docker doc, fix context var in run cmd #829
828: clean up docker doc, fix context var in run cmd #829
Conversation
@priyawadhwa - Issue that came up while working on #812. Let me know if this needs any changes. |
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.
@codevbus thanks for your contribution! Left a couple comments, please let me know if you have any questions.
@priyawadhwa - made changes based on your feedback. |
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.
README looks good, just one question.
@@ -35,9 +35,9 @@ if [[ ! -e $HOME/.config/gcloud/application_default_credentials.json ]]; then | |||
fi | |||
|
|||
docker run \ | |||
-v $HOME/.config/gcloud:/root/.config/gcloud \ | |||
-v ${context}:/workspace \ | |||
-v "$HOME"/.config/gcloud:/root/.config/gcloud \ |
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.
Are the changes to this file necessary?
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.
Most likely not, although it's generally safer to use "" in this case. Let me know if you want me to revert @priyawadhwa
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.
That's alright, let's keep this! Thanks.
LGTM thanks for adding this! |
Fixes #828. in case of a bug fix, this should point to a bug and any other related issue(s)
Description
${context}
var interpolation to run command.Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Reviewer Notes
Release Notes
Describe any changes here so maintainer can include it in the release notes, or delete this block.