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

gdrive: fix multi-remote workflow, cont. cleanup #3686

Merged
merged 13 commits into from
Apr 29, 2020
Merged

Conversation

shcheklein
Copy link
Member

@shcheklein shcheklein commented Apr 26, 2020

Address more checkboxes from the #2865.

Specifically:

  • We should store one credentials file per remote. Solution: behave optimistically, do not require a config option or complicate default naming (difficult names that depend on remote name may lead to discrepancies in simple scenarios like renaming or changing settings of a single GDrive remote). Better detect discrepancy, cases of access failure due to wrong credentials being used.
  • Fix how path for the gdrive_user_credentials_file is handled - it should be resolved against config location if it is relative.
  • Cleanup and simplify code in GDrive auth and other error handlers
  • Minor cleanup and typos fixed in a few other cases
  • Add tests

Docs PR: iterative/dvc.org#1192


  • ❗ I have followed the Contributing to DVC checklist.

  • 📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. If the CLI API is changed, I have updated tab completion scripts.

  • ❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@codecov
Copy link

codecov bot commented Apr 26, 2020

Codecov Report

Merging #3686 into master will decrease coverage by 0.11%.
The diff coverage is 71.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3686      +/-   ##
==========================================
- Coverage   90.77%   90.65%   -0.12%     
==========================================
  Files         158      158              
  Lines       10001    10018      +17     
==========================================
+ Hits         9078     9082       +4     
- Misses        923      936      +13     
Impacted Files Coverage Δ
dvc/remote/gdrive.py 80.07% <65.90%> (-3.40%) ⬇️
dvc/config.py 97.71% <100.00%> (ø)
dvc/data_cloud.py 97.05% <100.00%> (ø)
dvc/updater.py 54.65% <0.00%> (-1.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fefbcc0...0a1284a. Read the comment docs.

@shcheklein shcheklein changed the title gdrive: fix multi-remote workflow, cont. cleanup [WIP, ready to review] gdrive: fix multi-remote workflow, cont. cleanup Apr 27, 2020
@shcheklein shcheklein self-assigned this Apr 27, 2020
@shcheklein shcheklein added bug Did we break something? enhancement Enhances DVC labels Apr 27, 2020
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Questions and suggestions on cmd output.

dvc/remote/gdrive.py Show resolved Hide resolved
dvc/remote/gdrive.py Outdated Show resolved Hide resolved
dvc/remote/gdrive.py Outdated Show resolved Hide resolved
dvc/remote/gdrive.py Outdated Show resolved Hide resolved
dvc/remote/gdrive.py Outdated Show resolved Hide resolved
dvc/remote/gdrive.py Outdated Show resolved Hide resolved
@shcheklein shcheklein requested a review from jorgeorpinel April 28, 2020 03:04
@shcheklein shcheklein force-pushed the gdrive-cont-2865 branch 3 times, most recently from c9480fa to 238d6ed Compare April 28, 2020 17:55
dvc/remote/gdrive.py Outdated Show resolved Hide resolved
dvc/remote/gdrive.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

A few more details for your consideration ☝️

@shcheklein shcheklein changed the title [WIP, ready to review] gdrive: fix multi-remote workflow, cont. cleanup gdrive: fix multi-remote workflow, cont. cleanup Apr 29, 2020
dvc/remote/gdrive.py Outdated Show resolved Hide resolved
@efiop efiop merged commit 8aefbac into master Apr 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the gdrive-cont-2865 branch April 29, 2020 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Did we break something? enhancement Enhances DVC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants