-
Notifications
You must be signed in to change notification settings - Fork 38
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
Refactor Authorization docs (leaving stub pages behind) #1046
Conversation
- Move `authorization.rst` to `authorization/globus_authorizers.rst` - Create a stub at `authorization.rst` (orphan doc) which directs users to the new page - Introduce `authorization/index.rst` with a single-item TOC
- Move `scopes.rst` to `authorization/scopes_and_consents/scopes.rst` - Add `scopes.rst` as a stub, pointing to the new page - Reduce `experimental/scope_parser.rst` to a stub (was full doc!) - Move `MutableScope` docs to a separate page in `scopes_and_consents/` and note it as deprecated (doc note only) - Update `MutableScope` usage docs to use `Scope` instead - Update toctrees to include the new docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how much of this copy is new vs copy/pasted into a new location.
I like the new structure of it a lot but think that the page text itself needs some massaging to get to a consumable state. In the globus authorizers page in particular, we should structure to be as approachable as possible; there's a good chance, this is the first time that users are interacting with globus authorization.
.. toctree:: | ||
:maxdepth: 1 | ||
|
||
scopes | ||
mutable_scopes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to the top of the page (or signal in a more apparent way that this is a nav link structure).
Otherwise it looks at a glance to me like "these docs have been moved to docs.globus.org/guides/overview/clients_scopes_and_consents."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll rearrange. I don't know exactly how yet; let me get in there and see what looks right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I broke things apart into sections, so that the overview/background on scopes with the docs.globus.org link could go in a separate section. Take a look and LMK if we can mark this resolved!
In order to support optional and dependent scopes, an additional type is | ||
provided by ``globus_sdk.scopes``: the ``Scope`` class. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't frame the initial reference to Scopes as "for the purpose of optional and dependent scopes". I'd just say "this is the python class model for a scope" (and maybe that it happens to support optional and dependent scopes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good callout! That was adapted from text we have today about MutableScope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reworded, not only as you suggested but also to draw the connection that having such a model is useful for representing and manipulating dependent scopes. Take a look and see if it's satisfactory -- I think you'll agree that it's an improvement but maybe we need to refine it further.
.. code-block:: pycon | ||
|
||
>>> from globus_sdk.scopes import Scope | ||
>>> foo = Scope("foo") | ||
>>> bar = Scope("bar") | ||
>>> bar.add_dependency("baz") | ||
>>> foo.add_dependency(bar) | ||
>>> print(str(foo)) | ||
foo[bar[baz]] | ||
>>> print(bar.serialize()) | ||
bar[baz] | ||
>>> alpha = Scope("alpha") | ||
>>> alpha.add_dependency("beta", optional=True) | ||
>>> print(str(alpha)) | ||
alpha[*beta] | ||
>>> print(repr(alpha)) | ||
Scope("alpha", dependencies=[Scope("beta", optional=True)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use python
instead of pycon
for this code block for consistency across the page? Or is there an important reason to affiliate this with anaconda?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pycon is Python Console. It's more appropriate for this case because it contains output.
.. autodata:: globus_sdk.scopes.data.AuthScopes | ||
:annotation: | ||
|
||
.. autodata:: globus_sdk.scopes.data.FlowsScopes | ||
:annotation: | ||
|
||
.. autodata:: globus_sdk.scopes.data.GroupsScopes | ||
:annotation: | ||
|
||
.. autodata:: globus_sdk.scopes.data.NexusScopes | ||
:annotation: | ||
|
||
.. autodata:: globus_sdk.scopes.data.SearchScopes | ||
:annotation: | ||
|
||
.. note:: | ||
|
||
``TimersScopes`` is also available under the legacy name ``TimerScopes``. | ||
|
||
.. autodata:: globus_sdk.scopes.data.TimersScopes | ||
:annotation: | ||
|
||
.. autodata:: globus_sdk.scopes.data.TransferScopes | ||
:annotation: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These constants don't actually document the piece that I'd expect users to care about looking up.
The associated documentation for all of them is a duplicate (probably inherited) docstring & parameter reference. Could we instead list out the specific instance attributes here? e.g.:
class TransferScopes
all: Perform any transfer operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make that change, I'd also advocate that we move this up as far as we feel comfortable in the page; that seems like the most pertinent information (besides potential "how to use scopes" in this page) and right now it's at the bottom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we save debate about this for future changes? This is a lift and shift of existing docs which are built via the custom sphinx plugin. Changing this extensively is something I'm open to doing, but want to keep out of this scope of work.
Globus SDK clients need credentials (tokens) to authenticate against services | ||
and authorize their various API calls. Using Globus Auth, the Globus OAuth2 | ||
service, clients can be provided with credentials which are either static or | ||
dynamic. | ||
|
||
The interface used to handle these credentials is :class:`GlobusAuthorizer`. | ||
:class:`GlobusAuthorizer` defines methods which provide an ``Authorization`` | ||
header for HTTP requests, and different implementations handle Refresh Tokens, | ||
Access Tokens, Client Credentials, and their various usage modes. | ||
|
||
A :class:`GlobusAuthorizer` is an object which defines the following two | ||
operations: | ||
|
||
- get an ``Authorization`` header | ||
- handle a 401 Unauthorized error | ||
|
||
Clients contain authorizers, and use them to get and refresh their credentials. | ||
Whenever using the :ref:`Service Clients <services>`, you should be passing in an | ||
authorizer when you create a new client unless otherwise specified. | ||
|
||
The type of authorizer you will use depends very much on your application, but | ||
if you want examples you should look at the :ref:`examples section | ||
<examples-authorization>`. | ||
It may help to start with the examples and come back to the reference | ||
documentation afterwards. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This intro section contains too much detail.
All that needs to be conveyed (imo) is:
- All service clients use credentials (globus-auth tokens) to make requests.
- GlobusAuthorizer is an interface we added to make it simpler to do that right.
- [Maybe] A call to action of "read on where we explain how we do that and how to use it!"
An intro section should be as concise as possible, with as little technical detail as possible. The technical detail goes into the well defined section below where readers can read it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree on some specifics, but I generally agree that this section is too long. However, as above, this is a lift and shift with minimal edits, and I would like to keep it as such. I think a lot of this content needs to go somewhere and I would not be satisfied to simply remove it from the intro without finding an appropriate location for it.
Are you proposing a specific new place for that narrative documentation? I didn't move it because I didn't want to debate this element. If there's a place we can agree upon, I'm happy to move it. Otherwise, let's leave this in as is for the moment.
Co-authored-by: derek-globus <113056046+derek-globus@users.noreply.github.com>
for more information, see https://pre-commit.ci
Per conversation, basic requirements met.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm marking as "approved" to indicate my overall endorsement of the restructuring. I have a few modest change requests, but if you're okay with those suggestions, this puts you in a position to just apply and merge so we can keep things moving.
Co-authored-by: Ada <107940310+ada-globus@users.noreply.github.com>
authorization/
This is a preparatory move serving three purposes.
(1) It's just good cleanup.
(2) The new structure will let us document the
consents
module in a logical place when it moves out of experimental.(3) It makes room for GlobusApp, new tokenstorage docs, and other improvements.
📚 Documentation preview 📚: https://globus-sdk-python--1046.org.readthedocs.build/en/1046/