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

OIDC flow doc newbie improvements and big reorganization #33794

Merged
merged 6 commits into from
Jun 13, 2023

Conversation

emmanuelbernard
Copy link
Member

  • OIDC Doc - Improve OIDC documentation with specific newbie context
  • OIDC doc - Reorganize doc structure for easier flow and context
  • OIDC doc - Improve OAuth2 section
  • OIDC doc - Subsequent reorganization for easier read and context

The doc was fundamentally good, very detailed. Probably too detailed but it is a complex subject.
I added specific newbie context based on my conversations with Sergey.
Particularly the non-OIDC OAuth2 section.
I also reorganized the structure to group sections with alike subjects and created some uber sections to explain the context better.

Also lighten some sentences en passant
and split notes into Asciidoc based notes for more readability.
Group logout and expiration, group token validity subjects, create integration consideration section
Change TOC level for better navigation.
Based on exchanges with Sergey.
Create dedicated configuration uber section regrouping alike subjects (non code)
Move OAuth2 into Using the flow
Move PKCD into token validation section
Reorganize integration section and make it a top level
Move Tests to top level and fold error section
@emmanuelbernard
Copy link
Member Author

@sberyozkin this one is for you. :D
Big changes but I think for the better of readability.

@emmanuelbernard
Copy link
Member Author

@sberyozkin if you are time constrained, check the tactical changes I made (eg OAuth2) for their validity. The big structure change I'm fairly confident it is better than what it was before.

@github-actions
Copy link

github-actions bot commented Jun 2, 2023

🙈 The PR is closed and the preview is expired.

…ser-info property

In OAuth2 case, the latter is more correct.
@sberyozkin
Copy link
Member

Hi @emmanuelbernard Thanks for going through all of this document, getting some of the sections more visible, adding per section specific introductions and clarifications, and overall restructuring looks fine, it will help users. I've proposed a couple of minor suggestions, and it would be good to go IMHO.

My review will be easy though :-), but let me ask the super Doc OIDC team to have a look :-), @michelle-purcell @sheilamjones, can you please help with some feedback as well ?

Cheers

@emmanuelbernard
Copy link
Member Author

I've ap0plied your feedback @sberyozkin

@sberyozkin sberyozkin self-requested a review June 2, 2023 14:03
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

Thanks @emmanuelbernard.

Please merge once @michelle-purcell and @sheilamjones double check the refactored content, cheers

@michelle-purcell
Copy link
Contributor

@emmanuelbernard - Apologies for the delay in reviewing. Other priorities got in the way. I hope to complete this today. Thanks.

@inoxx03 inoxx03 self-requested a review June 12, 2023 13:33
@sberyozkin
Copy link
Member

Thanks for a super review @michelle-purcell @sheilamjones so far. Let me squash everything and then we can take care of the last few open comments, thanks

@sberyozkin
Copy link
Member

Hey @emmanuelbernard @michelle-purcell @sheilamjones

I've squashed all of the latest commits so the record looks like this:

https://github.com/quarkusio/quarkus/pull/33794/commits

There were more than hundred (I lost the count while squashing :-) ) commits, so Emmanuel, your last 2 commits, Apply Sergey's Feedback and Fixing relevant linting errors have been squashed with all the commits from Michelle and Sheila, since the linting errors one was essentially replaced by follow up commits, and my own feedback was very minor, so now we have all your main changes recorded and mention Michelle and Sheila, as well as mine contributions.

Hope that works for all :-), there were a couple of build failures related to NOTE, I think I fixed them.

@michelle-purcell Thanks for approving, if you have more comments let me know tomorrow please
@sheilamjones Thanks for all the suggestions too, if you can open suggested changes to the remaining items tomorrow then it would be great
I'll also have a look at some of the remaining items I wanted to contribute to.

Hey @inoxx03, I've noticed you are planning to review, it would be welcome :-). If we can resolve the remaining comments tomorrow with Michelle and Sheila then I'd not mind merging it, as it has already taken a lot of updates. So if you'd like to propose some significant changes, then please follow up with a PR once this PR is merged or open an issue, I can start with a follow up draft PR and you can propose changes... This PR can take more minor updates though if you'll have them proposed

Thanks Everyone

@sheilamjones
Copy link
Contributor

Thanks @sberyozkin and @emmanuelbernard for the opportunity to review these changes. It is a complex topic and the new content with additional context will be very helpful. My edits and suggestions are mostly to incorporate styling edits and some questions where I wasn't clear on the intended meaning. Thanks @sberyozkin for incorporating the suggestions so far and if there are any questions or wish to discuss any feedback, please feel free to ping.
Kind regards,
Sheila

@michelle-purcell
Copy link
Contributor

@sberyozkin - Thank you. Super job keeping this one on the tracks 🚆 🦸 and +1 to all of the above and getting this great work merged today.

I suggest we handle any new suggestions from you @inoxx03 and others in further smaller PRs. In parallel, I am also currently reviewing and editing this content too in preparation for consumption downstream so we will have at least one more iteration before the 3.2 release freeze.

@sberyozkin
Copy link
Member

Thanks @michelle-purcell, let me work with the last few open comments and I'll ping you and @sheilamjones to give it one last quick check before merging, cheers

@sberyozkin
Copy link
Member

By the way, the build failure is the one I was referring to. I did get it fixed but forgot to commit :-)

@sberyozkin sberyozkin marked this pull request as ready for review June 13, 2023 14:09
@sberyozkin
Copy link
Member

Hey @michelle-purcell @sheilamjones I've tried to resolve all the open comments. Have a look please if you'll have a few minutes, I'm hoping to merge once the build passes. The plan is to continue improving these docs anyway so we'll handle whatever might have been missed (including by myself while dealing with your comments :-) ) later in any case :-)

@sheilamjones
Copy link
Contributor

Thank you @sberyozkin for addressing the comments and feedback so promptly. I will take a look again now.

@sheilamjones
Copy link
Contributor

Thank so much @sberyozkin for your detailed review and all your work addressing the comments. There are some open that are still pending in the latter sections of the topic, but you might want to address those in a next iteration. Aside from those, I don't have any further feedback. Many thanks,

@sberyozkin
Copy link
Member

Hey @sheilamjones Thanks for checking it. I've just tried 3 times and I don't see any open comments - but it is likely, due to a large number of changes, I keep missing them. I guess it is indeed the right time to merge this PR then :-), we can take care of keeping tuning the doc during the next iterations.
Thanks again @sheilamjones @michelle-purcell 👍

@sberyozkin sberyozkin merged commit 5c2df51 into quarkusio:main Jun 13, 2023
@quarkus-bot quarkus-bot bot added this to the 3.2 - main milestone Jun 13, 2023
@michelle-purcell
Copy link
Contributor

@sberyozkin - LGTM. Thank you 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants