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

improve authorization configuration #866

Merged

Conversation

alexandra-simeonova
Copy link
Contributor

Description

Changes proposed in this pull request:

  • add a TOC
  • fix grammar errors

Related issue(s)
#865

@alexandra-simeonova alexandra-simeonova force-pushed the improve-authorization-doc branch from a4d80d9 to 46b37a5 Compare October 7, 2019 11:26
Copy link
Contributor

@zarkosimic zarkosimic left a comment

Choose a reason for hiding this comment

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

LGTM!


You have the following authorization options:
* [OpenID Connect](#openid-connect-configuration)
* [Third-party cookies and silent token refresh](#third-party-cookies-and-silent-token-refresh)

Choose a reason for hiding this comment

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

Suggested change
* [Third-party cookies and silent token refresh](#third-party-cookies-and-silent-token-refresh)
* [Third-party cookies and silent token refresh](#third-party-cookies-and-silent-token-refresh)

Is this indentation intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, because that's supposed to be a sub-category of OpenID connect (that's also why its heading begins with ### instead of ##)

Choose a reason for hiding this comment

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

Oh, right. Please ignore it then :)

docs/authorization-configuration.md Outdated Show resolved Hide resolved
docs/authorization-configuration.md Outdated Show resolved Hide resolved
@@ -87,11 +107,11 @@ auth: {
- **expirationCheckInterval** the number of seconds to pass between each check if the token is about to expire. The default value is `5` seconds.


### Custom Authentication Provider
## Another authorization provider

Choose a reason for hiding this comment

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

Why the switch from Custom to Another? I think another (or, alternatively, (a) different) sounds better, as another might give an impression as if there were more than one in use at the same time. What is more, you still use the word custom and it's used in code (see lines 115, 143) so I'd stick with custom for consistency's sake if it's all the same.
(Applies also to line 112.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I changed it 👍

docs/authorization-configuration.md Outdated Show resolved Hide resolved

If you are using any authentication provider you can also implement the following functions for Luigi.
If you use a custom authorization provider, you can also implement these functions for Luigi.

Choose a reason for hiding this comment

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

Suggested change
If you use a custom authorization provider, you can also implement these functions for Luigi.
If you use a custom authorization provider, you can also implement these functions for Luigi:

@JohannesDoberer JohannesDoberer modified the milestones: Sprint 6, Sprint 7 Oct 14, 2019
@alexandra-simeonova alexandra-simeonova merged commit 2e08ec1 into SAP:master Oct 15, 2019
@alexandra-simeonova alexandra-simeonova deleted the improve-authorization-doc branch October 15, 2019 13:31
stanleychh pushed a commit to stanleychh/luigi that referenced this pull request Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation documentation tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants