-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
docs: update and enhance oidc documentation #2142
docs: update and enhance oidc documentation #2142
Conversation
Thanks for choosing to contribute @georglauterbach. We lint all PR's with golangci-lint and eslint, I may add a review to your PR with some suggestions. You are free to apply the changes if you're comfortable, alternatively you are welcome to ask a team member for advice. ArtifactsThese changes once approved by a team member will be published for testing on Buildkite, DockerHub and GitHub Container Registry. Docker Container
|
Not sure whether there is a documentation preview set up for this project. If there is, please provide me with a link so I can have a look at a deployed version myself. Please have an maintainer label this accordingly and put it into the OIDC project :) PS: Sorry for the force-push (I know, it's discouraged). Had to do it because of the GPG signature (changed recently and wasn't updated properly). |
No worries! We mainly prefer it to keep a reasonable history of the progression of a PR if necessary.
There currently isn't, but we have a JTD testing area which I can push these changes to and you can see how it looks. We're probably going to have to discuss some of these changes internally since they differ from a thorough configuration doc restyle recently. I'm about to sleep so will take a closer look in about 9 hours. |
SGTM 👍🏼
Alright, I already anticipated that there is some sort of style guideline. I may have overlooked them. But of course, these changes are open for discussion. Just reach back to me :) |
https://authelia.github.io/jtd-test/?random=1231278978744 Here's a preview of that page with some easy to notice differences. Grafana has also been tested and works as expected. |
Nice! I noticed some small mistakes that I will correct soon. Won‘t have time on the weekend though. Reach back to me if there are style adjustments or things you’d like to see changed. I will most likely be able to correct them on Monday. |
Co-authored-by: Clément Michaud <clement.michaud34@gmail.com>
Co-authored-by: Clément Michaud <clement.michaud34@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #2142 +/- ##
==========================================
+ Coverage 69.63% 69.73% +0.09%
==========================================
Files 176 176
Lines 5148 5148
Branches 200 200
==========================================
+ Hits 3585 3590 +5
+ Misses 1287 1282 -5
Partials 276 276
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@james-d-elliott @clems4ever Ready for review with suggestions incorporated. I know that the command / pipe for creating the secrets (especially in K8s) seems overkill, but this solution does not rely on |
Resolved the conflicts, this is ready again to be merged :) |
Thanks! I'll take a look today. We've all been very busy sorry. |
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.
Mostly LGTM, few suggestions.
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.
There's a small typo in your latest update.
Co-authored-by: Amir Zarrinkafsh <nightah@me.com>
Co-authored-by: Amir Zarrinkafsh <nightah@me.com>
Co-authored-by: James Elliott <james-d-elliott@users.noreply.github.com>
Co-authored-by: James Elliott <james-d-elliott@users.noreply.github.com>
Thanks for the type hint @nightah. @james-d-elliott I incorporated your suggestions :) |
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 like the changes you've made so far!
Only few tiny remaining things to fix.
Co-authored-by: Clément Michaud <clement.michaud34@gmail.com>
Co-authored-by: Clément Michaud <clement.michaud34@gmail.com>
Co-authored-by: Clément Michaud <clement.michaud34@gmail.com>
Co-authored-by: Clément Michaud <clement.michaud34@gmail.com>
Co-authored-by: Clément Michaud <clement.michaud34@gmail.com>
Co-authored-by: Clément Michaud <clement.michaud34@gmail.com>
Co-authored-by: Clément Michaud <clement.michaud34@gmail.com>
Co-authored-by: Clément Michaud <clement.michaud34@gmail.com>
Co-authored-by: Clément Michaud <clement.michaud34@gmail.com>
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.
LGTM
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.
LGTM! just need to merge master into it so we can merge this. Typically we recommend people give us access when they create a PR (so we can easily merge), though it's not required. Cancel that, you did give us access.
default: 30d | ||
{: .label .label-config .label-blue } | ||
required: no | ||
{: .label .label-config .label-green } | ||
</div> | ||
|
||
The maximum lifetime of a refresh token. This should typically be slightly more the other token lifespans. This is |
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.
Not sure why the comment about how long the token should be was removed. However I can just add it back later in another paragraph. Other than that LGTM!
Since there is no PR template - strangely enough - I will use the one we use over at docker-mailserver/docker-mailserver. Feel free to copy it :).
Description
This PR enhances, extends and updates the OIDC section in the documentation. Changes include
Context: #189
Type of change
Checklist:
docs/
)