Support configuring the author email address for git. #1128
Conversation
With this change, Datalab will configure git with an author email address at startup. That can also be overridden via an environment variable.
| @@ -27,3 +27,6 @@ export CLOUDSDK_CONFIG=/content/datalab/.config | |||
| export PROJECT_ID=${PROJECT_ID:-`gcloud config list -q --format 'value(core.project)' 2> /dev/null`} | |||
| export ZONE=${ZONE:-`gcloud config list -q --format 'value(compute.zone)' 2> /dev/null`} | |||
|
|
|||
| # Lookup the author email address to use for git commits, and then configure git accordingly | |||
| export GIT_AUTHOR=${DATALAB_GIT_AUTHOR:-`gcloud auth list --format 'value(account)' --filter 'status:ACTIVE'`} | |||
There was a problem hiding this comment.
Should this fail instead of using the service account for git, which is a bad config in this case?
There was a problem hiding this comment.
I don't agree that using the service account is a bad config.
It may be ugly, but it is technically correct. Additionally, advanced users may create descriptively named service accounts for which the service account is actually the best email address to use.
More importantly, we don't expect that case to actually happen.
Users will usually use the CLI to create Datalab instances running in GCE VMs, and that tool will set the right environment variable.
That means the case where this will happen is for someone running Datalab locally, and in that case their gcloud credentials should be the right email address to use.
| @@ -93,6 +98,10 @@ | |||
| environment variable CLOUDSDK_COMPUTE_ZONE. | |||
| """) | |||
|
|
|||
| _EMAIL_HELP = ("""The email address associated with the instance. | |||
|
|
|||
| This should represent the user of Datalab.""") | |||
There was a problem hiding this comment.
I think we should add something about using the email for git here, otherwise it's not clear why this is suddenly required
| subcommand_parser.add_argument( | ||
| '--email', | ||
| dest='email', | ||
| default=get_email_address(), |
There was a problem hiding this comment.
Shouldn't this be a required argument instead of using service account?
There was a problem hiding this comment.
This wouldn't default to a service account unless the user runs datalab create from inside of a VM, and that seems like a rare scenario.
| @@ -170,6 +192,12 @@ def run(): | |||
| dest='zone', | |||
| default=None, | |||
| help=_ZONE_HELP) | |||
| if command_config['require-email']: | |||
| subcommand_parser.add_argument( | |||
| '--email', | |||
There was a problem hiding this comment.
A question:
- Is this only for git user -- if so git-user instead of email is likely much more descriptive/appropriate.
Thought:
- Do we actually need this? Won't the default always suffice, esp. with cloud source repository?
There was a problem hiding this comment.
This is just a temporary solution I am using to test the changes to the Docker image.
This change isn't ready for review yet.
There was a problem hiding this comment.
FYI, this is ready for review now.
There was a problem hiding this comment.
I think my comment still holds ... about this being worded as email in the flags, as opposed to git-user or something that indicates use.
There was a problem hiding this comment.
To clarify, the '--email' flag is now gone.
This improves that in two ways: 1. Only use a single environment variable (DATALAB_GIT_AUTHOR) instead of two (DATALAB_GIT_AUTHOR and GIT_AUTHOR) 2. Set the user.name config in addition to the user.email one.
This change switches from passing around the user's email address via an overridable '--email' command line flag to instead using a fixed keyword argument. This has the consequence of the user no longer being able to override the email address, but the benefit of our command line surface being smaller and simpler.
| @@ -293,7 +293,7 @@ def maybe_start(args, gcloud_compute, instance): | |||
| return | |||
|
|
|||
|
|
|||
| def run(args, gcloud_compute): | |||
| def run(args, gcloud_compute, **unused_kwargs): | |||
There was a problem hiding this comment.
what does unused_kwargs add here if we're not passing it around?
There was a problem hiding this comment.
Keyword args are being passed in. There has to be a corresponding **something parameter, or that call will fail.
| @@ -170,6 +192,12 @@ def run(): | |||
| dest='zone', | |||
| default=None, | |||
| help=_ZONE_HELP) | |||
| if command_config['require-email']: | |||
| subcommand_parser.add_argument( | |||
| '--email', | |||
There was a problem hiding this comment.
I think my comment still holds ... about this being worded as email in the flags, as opposed to git-user or something that indicates use.
With this change, Datalab will configure git with an author email
address at startup. That can also be overridden via an environment
variable.