Skip to content
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

fs.gdrive: Fix GDRIVE_CREDENTIALS_DATA for service accounts #7213

Merged
merged 3 commits into from
Apr 8, 2022
Merged

fs.gdrive: Fix GDRIVE_CREDENTIALS_DATA for service accounts #7213

merged 3 commits into from
Apr 8, 2022

Conversation

RCdeWit
Copy link
Contributor

@RCdeWit RCdeWit commented Dec 30, 2021

_validate_config() had a bug: it would not take into account that Google
Drive credentials can also be passed using GDRIVE_CREDENTIALS_DATA as
documented here 1.

This patches the validation logic to also accept this environment variable as a
means to provide service account credentials.

Before, commands like dvc push would exit with:

ERROR: failed to push data to the cloud - To use service account, set
  `gdrive_service_account_json_file_path`, and
  optionally`gdrive_service_account_user_email` in DVC config
Learn more at <https://man.dvc.org/remote/modify>

Thanks to @duijf for helping me diagnose.

Co-authored-by: Laurens Duijvesteijn [email protected]

@RCdeWit RCdeWit requested a review from a team as a code owner December 30, 2021 18:57
@RCdeWit RCdeWit requested a review from karajan1001 December 30, 2021 18:57
@pmrowla pmrowla added bugfix fixes bug fs: gdrive Related to the GDrive filesystem labels Jan 3, 2022
@pmrowla
Copy link
Contributor

pmrowla commented Jan 3, 2022

Related: iterative/dvc-gdrive#20

dvc/fs/gdrive.py Outdated Show resolved Hide resolved
dvc/fs/gdrive.py Outdated
Comment on lines 118 to 119
"You can also provide these using the GDRIVE_CREDENTIALS_DATA"
"environment variable.\n"
Copy link
Member

Choose a reason for hiding this comment

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

I think we can just leave this message out.

@dberenbaum
Copy link
Collaborator

@RCdeWit Thanks for the contribution! Would you be able to respond to update the PR to address the comments?

@RCdeWit
Copy link
Contributor Author

RCdeWit commented Mar 10, 2022

Thanks for the review, folks. Incorporated the feedback!

Copy link
Contributor

@daavoo daavoo left a comment

Choose a reason for hiding this comment

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

Not saying it should block the P.R. but we should probably test for this (maybe in https://github.com/iterative/dvc/blob/30d6839a5f471fb02a7a2dcbd6a6616b71277762/tests/unit/remote/test_gdrive.py)

@RCdeWit
Copy link
Contributor Author

RCdeWit commented Apr 8, 2022

I'm working on part 2 of a blog post that covers this use case. I was hoping to release it somewhere over the next two weeks; any chance this fix will be merged by then? If not, my blog post will have to include the workaround.

# GDRIVE_CREDENTIALS_DATA is not read automatically when using a service account
# Manually write it to creds.json and read that file
echo -E '${{ secrets.GDRIVE_CREDENTIALS_DATA }}' > creds.json

dvc remote add -d -f myremote gdrive://${{ secrets.GOOGLE_DRIVE_URI }}
dvc remote modify myremote gdrive_use_service_account true
dvc remote modify myremote --local gdrive_service_account_json_file_path creds.json

dvc push

# Either do this or add the file to your .gitignore
# Just make sure not to push it to your repository
rm creds.json

Rob de Wit and others added 3 commits April 8, 2022 17:24
`_validate_config()` had a bug: it would not take into account that Google
Drive credentials can also be passed using `GDRIVE_CREDENTIALS_DATA` as
documented here [1].

This patches the validation logic to also accept this environment variable as a
means to provide service account credentials.

Before, commands like `dvc push` would exit with:

```
ERROR: failed to push data to the cloud - To use service account, set
  `gdrive_service_account_json_file_path`, and
  optionally`gdrive_service_account_user_email` in DVC config
Learn more at <https://man.dvc.org/remote/modify>
```

[1]: https://dvc.org/doc/user-guide/setup-google-drive-remote#authorization

Co-authored-by: Laurens Duijvesteijn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix fixes bug fs: gdrive Related to the GDrive filesystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants