Skip to content
This repository has been archived by the owner on Sep 3, 2022. It is now read-only.

Support configuring the author email address for git. #1128

Merged
merged 5 commits into from
Jan 19, 2017
Merged

Support configuring the author email address for git. #1128

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c560e82
Support configuring the author email address for git.
ojarjur Jan 14, 2017
feb6999
Merge branch 'master' into ojarjur/startup-script
ojarjur Jan 18, 2017
9cee4d3
Clean up the setting of the git user config
ojarjur Jan 18, 2017
b841495
Remove the --email flag from the CLI
ojarjur Jan 18, 2017
65b864f
Fill in the docstring for the get_email_address method
ojarjur Jan 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions containers/datalab/content/setup-env.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'`}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this fail instead of using the service account for git, which is a bad config in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

git config --global user.email "${GIT_AUTHOR}"
4 changes: 3 additions & 1 deletion tools/cli/commands/create.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@
value: GCE
- name: DATALAB_SETTINGS_OVERRIDES
value: '{{"enableAutoGCSBackups": {1} }}'
- name: DATALAB_GIT_AUTHOR
value: '{2}'
volumeMounts:
- name: home
mountPath: /content
Expand Down Expand Up @@ -380,7 +382,7 @@ def run(args, gcloud_compute):
startup_script_file.close()
manifest_file.write(
_DATALAB_CONTAINER_SPEC.format(
args.image_name, enable_backups))
args.image_name, enable_backups, args.email))
manifest_file.close()
metadata_from_file = (
'startup-script={0},google-container-manifest={1}'.format(
Expand Down
28 changes: 28 additions & 0 deletions tools/cli/datalab.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,15 @@
'flags': create.flags,
'run': create.run,
'require-zone': True,
'require-email': True,
},
'connect': {
'help': 'Connect to an existing Datalab instance',
'description': connect.description,
'flags': connect.flags,
'run': connect.run,
'require-zone': True,
'require-email': False,
},
'list': {
'help': 'List the existing Datalab instances in a project',
Expand All @@ -49,20 +51,23 @@
'flags': list.flags,
'run': list.run,
'require-zone': False,
'require-email': False,
},
'stop': {
'help': 'Stop an existing Datalab instance',
'description': stop.description,
'flags': stop.flags,
'run': stop.run,
'require-zone': True,
'require-email': False,
},
'delete': {
'help': 'Delete an existing Datalab instance',
'description': delete.description,
'flags': delete.flags,
'run': delete.run,
'require-zone': True,
'require-email': False,
},
}

Expand Down Expand Up @@ -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.""")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add something about using the email for git here, otherwise it's not clear why this is suddenly required



try:
with open(os.devnull, 'w') as dn:
Expand Down Expand Up @@ -128,6 +137,19 @@ def gcloud_compute(
cmd, stdin=stdin, stdout=stdout, stderr=stderr)


def get_email_address():
"""Get the email address of the user's active gcloud account.

Returns:

Raises:
subprocess.CalledProcessError: If the gcloud command fails
"""
return subprocess.check_output([
gcloud_cmd, 'auth', 'list', '--quiet', '--format',
'value(account)', '--filter', 'status:ACTIVE'])


def run():
"""Run the command line tool."""
parser = argparse.ArgumentParser(
Expand Down Expand Up @@ -170,6 +192,12 @@ def run():
dest='zone',
default=None,
help=_ZONE_HELP)
if command_config['require-email']:
subcommand_parser.add_argument(
'--email',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this is ready for review now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, the '--email' flag is now gone.

dest='email',
default=get_email_address(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be a required argument instead of using service account?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

help=_EMAIL_HELP)

args = parser.parse_args()
try:
Expand Down