quarkus
quarkus copied to clipboard
Docs: Enhancements to OIDC introduction in Security docs
JIRA: QDOCS-31
Edits to the Quarkus security content to enhance introduction to OpenID Connect
@hmanwani-rh: Hi Heena, Can you please review this initial draft PR this week? I updated the existing OpenID Connect section of the Security Architecture and Guides content to add an introduction to OIDC and incorporate style edits.
Many thanks Sheila
Hi @sheilamjones It looks nice, I've added a few more minor suggestions - lets try to resolve/discuss them and then I'll commit this PR and then look at integrating the diagram into the code flow guide, cheers
Hi @sheilamjones It LGTM now, however the log history seems to have gone wrong, I've checkout your branch, and I see
commit c80628f1ed (HEAD -> QDOCS-31-OIDC-INTRO, origin/QDOCS-31-OIDC-INTRO)
Merge: 61b261f6a2 b7f85ce220
Author: sheilamjones <[email protected]>
Date: Mon Aug 29 20:50:02 2022 +0100
Merge branch 'main' into QDOCS-31-OIDC-INTRO
commit 61b261f6a2
Merge: 209ed32b9d 9ce8749a20
Author: shjones <[email protected]>
Date: Mon Aug 29 15:17:41 2022 +0100
Merge branch 'main' of https://github.com/quarkusio/quarkus into QDOCS-31-OIDC-INTRO
commit b7f85ce220 (origin/main, origin/HEAD, main)
Merge: 547eb817d0 15068cd00d
Author: Guillaume Smet <[email protected]>
Date: Mon Aug 29 20:12:09 2022 +0200
Merge pull request #27577 from manovotn/explicitDepOnPropagators
Add explicit dependency on smallrye-context-propagation-propagators-rxjava2
....
In such cases Guillaume recommends making a copy of the branch, resetting hard the content of the current branch and cherry-picking the individual commits (if I got it right :-)), but I'm not sure I'm seeing in the commit sequence all the commits now.
I've looked at the actual OIDC introduction section and it does look good to me now. So may be a simpler option is to create a new branch off main, and replace the OIDC introduction section in the new branch with the modified OIDC introduction section from this PR, then close this PR and open a new PR from that branch - it should be OK I guess.
@gsmet Does it sound right to you ?
Thanks
Hi @sberyozkin, thanks for your proposal. I'm not sure what has happened with some of the commit history. Thanks for your suggested fix recommendations. Would it be okay @gsmet to fix per Sergey's proposal to create a new branch off main and update the section again in the new branch with the modifications from this PR?
@sberyozkin, @gsmet: I worked with Stefan to rectify the issues with my commit history. I had initiated a recursive commit by using the Github GUI to update my branch. To fix, we made a copy of the branch, issued a hard reset and cherry-picked the individual commits. I went through the updated content again and verified that all the changes are present.
@sheilamjones Hi Sheila, apologies for a delay, I've missed your update. I've squashed the commits, and had to resolve the conflict since Michal's @michalvavrik PR affecting the same section was merged this morning. To resolve the conflict I've moved the sentences related to Keycloak admin client and centralized authorization out of the note, and used the pattern you have used For more information about..., see ....
Specifically, the new admin guide added by Michal has the info about the dependencies/etc so it was not really needed at the top level security.adoc any longer. Likewise I noticed while resolving the conflict that the intro about KC and Centralized Authorization was reworded like "for more info about KC and bearer tokens" which I thought was not quite right.
Hope you are OK with it, have a look please, we can tweak a few more things if you'd like, it is ready for merge now, from my point of view as well
@sheilamjones Let me merge it and as I said we can always add a few more updates easily. Thanks for this PR, look forward to seeing the PR to the OIDC web-app guide :-)
@sberyozkin: Thanks so much for your review and merge Sergey and for your help with this. Apologies, I didn't see the earlier notification until now, but the additional tweaks to facilitate the conflict resolution look good to me.
@sheilamjones np at all :-), thanks for double checking the last updates