Skip to content

cmd: update remote modify with --local as needed#1607

Merged
shcheklein merged 4 commits into
masterfrom
cmd/remote/modify-1603
Jul 22, 2020
Merged

cmd: update remote modify with --local as needed#1607
shcheklein merged 4 commits into
masterfrom
cmd/remote/modify-1603

Conversation

@jorgeorpinel
Copy link
Copy Markdown
Contributor

per #1603 (review)

You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please choose to allow us to edit your branch when creating the PR.

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

@jorgeorpinel jorgeorpinel requested review from efiop and shcheklein July 21, 2020 03:43
@shcheklein shcheklein temporarily deployed to dvc-landing-cmd-remote--f1jztd July 21, 2020 03:43 Inactive

```dvc
$ dvc remote modify myremote gdrive_client_id <client ID>
$ dvc remote modify --local myremote gdrive_client_id <client ID>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

these are fine to be shared, everything for GDrive is more or less fine to share

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

True, I didn't think this one through. Updated.

But I think it's safer to leave gdrive_user_credentials_file as --local. Even when the path itself is not THE secret info, in strict terms paths to private files are considered private as well. This is why I included other paths similarly, but in the paragraph for those I say "may contain sensitive user info", depending on each case.

Comment thread content/docs/command-reference/remote/modify.md Outdated
Copy link
Copy Markdown
Contributor

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

Looks great, but we went a bit too far with - some fields are fine to share.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-cmd-remote--f1jztd July 21, 2020 20:34 Inactive
@jorgeorpinel
Copy link
Copy Markdown
Contributor Author

jorgeorpinel commented Jul 21, 2020

some fields are fine to share

I erred on the side of caution for some (as explained in #1607 (comment)).

Or should I roll back adding --local from all those indirectly private settings?

Comment thread content/docs/command-reference/remote/modify.md Outdated
Comment thread content/docs/command-reference/remote/modify.md Outdated
Comment thread content/docs/command-reference/remote/modify.md Outdated
Comment thread content/docs/command-reference/remote/modify.md Outdated
Comment thread content/docs/command-reference/remote/modify.md Outdated
Comment thread content/docs/command-reference/remote/modify.md Outdated
@shcheklein
Copy link
Copy Markdown
Contributor

@jorgeorpinel left some comments in the PR to revert some --local instances.

@jorgeorpinel jorgeorpinel requested a review from shcheklein July 22, 2020 02:37
@shcheklein
Copy link
Copy Markdown
Contributor

@jorgeorpinel forgot to push? :)

@jorgeorpinel
Copy link
Copy Markdown
Contributor Author

jorgeorpinel commented Jul 22, 2020

Yes 🤦 Ready...

$ dvc remote modify --local myremote password mypassword
```

> The username, port, and password (may) contain sensitive user info. Therefore,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

port is not a secret as well

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could be! But OK, removed in 32c775b.

Copy link
Copy Markdown
Contributor

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

one more item ... sorry missed it last time

@jorgeorpinel jorgeorpinel requested a review from shcheklein July 22, 2020 03:14
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-cmd-remote--f1jztd July 22, 2020 03:14 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants