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

Authentication examples for Azure Blob Storage #2634

Merged
merged 10 commits into from
Jul 28, 2021

Conversation

meierale
Copy link
Contributor

Added some examples how to remote modify dvc for various azure blob storage authentication scenarios.

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. 🙏

Added some examples how to remoge modify dvc for various azure blob storage authentication scenarios.
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.

Thanks @meierale ! I have a few questions on possibly unrelated/unnecessary changes for now (@isidentical will take a look at the examples when possible).

content/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
content/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
content/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
minor adjustments due to feedback
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.

Copy edits (committing) ☝️

Comment on lines 414 to 416
$ dvc remote add -d myremote azure://mycontainer/object
$ dvc remote modify --local myremote connection_string 'storageaccountaccesskeyconnectionstring'
$ dvc remote push
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait... We already have these examples under each config option. E.g.

image

The hope is that the remote add and push/pull commands are obvious to people reading this section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My point (and thus I included the dvc remote push command) is that I wanted to provide examples of correctly working combinations of authentication settings.

The already existing examples only show what parameters can be added, but not in which way they should be combined.

Copy link
Contributor

Choose a reason for hiding this comment

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

not in which way they should be combined

The ways in which they should be combined is there, pretty close to the beginning of this section:

image

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.

More copy edits for now (committing) 👇

content/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
content/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
content/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
content/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
@jorgeorpinel jorgeorpinel removed the request for review from isidentical July 19, 2021 08:09
@jorgeorpinel
Copy link
Contributor

@meierale thanks a lot for this contribution. I'm not sure what information we're adding though. Full usage examples, I guess? Was it pretty hard to put together remote add and remote modify (+ push/pull) from existing docs?

If we are to keep these I would definitely move to a proper example after https://dvc.org/doc/command-reference/remote/modify#example-customize-an-s3-remote. But at the moment I incline to wait as it should be addressed along with #1859

@shcheklein
Copy link
Member

My 2cs - we should merge this. These examples look very useful, since Azure is the most convoluted provider of those we support. It looks like a good contribution. If/when we have a full page we can move it. At the moment, this information probably is even more useful than everything we had before for Azure :)

meierale added 2 commits July 23, 2021 12:53
change L347 back to how it originally was.
make `az cli` credential example the first example, add note about contributor role.
@shcheklein
Copy link
Member

@meierale thanks for the PR

@jorgeorpinel any concerns merging this :)?

@jorgeorpinel jorgeorpinel self-assigned this Jul 28, 2021
@jorgeorpinel jorgeorpinel requested review from jorgeorpinel and removed request for jorgeorpinel July 28, 2021 06:02
@jorgeorpinel
Copy link
Contributor

any concerns merging this :)?

Yes @shcheklein , the Azure section is long already and with these examples it seems unreadable to me (I checked on the dev app locally). They're also somewhat repetitive and incomplete.

I understand the value of having the combinations somewhere, but the ref of remote add already has the full example of what we consider most used. Maybe we can add a couple of these in there instead? But are we going to do the same for other remote types?

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.

For now I moved this to an actual example in this same page and copy edited, but I still think #1859 will take care of this properly... Feel free to merge though. Thanks!

Copy link
Contributor

@isidentical isidentical left a comment

Choose a reason for hiding this comment

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

LGTM as well!

@shcheklein
Copy link
Member

Can we add a link to these examples from the add/modify Azure sections ?

@jorgeorpinel
Copy link
Contributor

Linked from remote add/modify refs! Ok merging this for now 🙂

Thanks again @meierale !

@jorgeorpinel jorgeorpinel merged commit 5a9e51f into iterative:master Jul 28, 2021
@meierale meierale deleted the patch-1 branch July 28, 2021 18:37
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.

4 participants