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.

Sign up for GitHub

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

Jump to bottom

Enhancements for the JPA-related content #31297

Merged
merged 1 commit into from
Feb 20, 2023
Merged

Enhancements for the JPA-related content #31297

merged 1 commit into from
Feb 20, 2023

Conversation

MichalMaler
Copy link
Contributor

@MichalMaler MichalMaler commented Feb 20, 2023 •
edited
Loading

Enhancements for the JPA-related content:

  • Removing concept-type information out of the JPA tutorial and creating a new JPA concept module.
  • Adding xrefs to related documentation.
  • Minor style tweaks in the Quarkus security documentation.

Main files for downstreaming:

security-jpa-concept.adoc
security-basic-authentication-tutorial.adoc
security-identity-providers-concept.adoc
security-authentication-mechanisms-concept.adoc

Closing: https://issues.redhat.com/browse/QDOCS-99

@MichalMaler MichalMaler force-pushed the QDOCS-99-Revamping-the-JPA-section branch from c191da7 to 9267820 Compare February 20, 2023 12:02
@MichalMaler MichalMaler requested review from sberyozkin and michelle-purcell and removed request for sberyozkin February 20, 2023 12:03
@sberyozkin
Copy link
Member

@MichalMaler I think it is an excellent write-up, thanks, nicely refactored.
My only ask is to copy (not move) to this new JPA concept doc this section as well, https://quarkus.io/guides/security-basic-authentication-tutorial#define-the-user-entity

What you have moved to the JPA concept is an extra content in addition to what is covered in https://quarkus.io/guides/security-basic-authentication-tutorial#define-the-user-entity, so copying it will make the JPA concept doc complete

@github-actions
Copy link

github-actions bot commented Feb 20, 2023 •
edited
Loading

🙈 The PR is closed and the preview is expired.

@MichalMaler MichalMaler force-pushed the QDOCS-99-Revamping-the-JPA-section branch 2 times, most recently from 33dc8b5 to 6ddbc78 Compare February 20, 2023 13:00
@MichalMaler
Copy link
Contributor Author

@sberyozkin Hello Siarhei! Added all the items from your wish list.
@michelle-purcell Michelle, can I have your opinion on newly added content? Much thanks for your expert review :)

@MichalMaler
Copy link
Contributor Author

@jmartisk Thanks Jan for the suggestions and additional SME review during the drafting phase.

@MichalMaler MichalMaler force-pushed the QDOCS-99-Revamping-the-JPA-section branch from 6ddbc78 to c4d1448 Compare February 20, 2023 14:11
sberyozkin
sberyozkin approved these changes Feb 20, 2023
View reviewed changes
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.

LGTM, lets wait for a bit longer before merging in case more review comments will follow

@MichalMaler MichalMaler force-pushed the QDOCS-99-Revamping-the-JPA-section branch from 57f0b96 to 1ae524c Compare February 20, 2023 16:11
@MichalMaler
Copy link
Contributor Author

@michelle-purcell Thank you for the review! Applied lot of great feedback.
@sberyozkin , please, can you go over last two Michelle's suggestion and provide a correct version of here fix? I like the style correction, however, I am not sure if it didn't break the technical meaning.

Please, put your OK tho the last two comments or provide an accurate formulation. Will apply it and do the last squash/rebase combo.

Cheers.

@sberyozkin
Copy link
Member

@MichalMaler Hi Michal, the last 2 comments from Michelle look fine to me

@MichalMaler MichalMaler force-pushed the QDOCS-99-Revamping-the-JPA-section branch from 4c4ae9d to 8597173 Compare February 20, 2023 16:33
@MichalMaler
Copy link
Contributor Author

@sberyozkin Cool! Applied.
My job is done here.
Cheers!

michelle-purcell
michelle-purcell approved these changes Feb 20, 2023
View reviewed changes
Copy link
Contributor

@michelle-purcell michelle-purcell left a comment

Choose a reason for hiding this comment

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

@MichalMaler - Super job. Thank you. Looks great 💯 See my style comments ⬆️ .

[I completed my review. Apologies, my laptop is dead so I had to find another machine to finish the review!]

@MichalMaler @michelle-purcell
Enhancements for the JPA-related content
bc7c502 
Signed-off-by: Michal Maléř <[email protected]>

Adding additional content

Signed-off-by: Michal Maléř <[email protected]>

Adding references

Signed-off-by: Michal Maléř <[email protected]>

Fixes

Signed-off-by: Michal Maléř <[email protected]>

Apply suggestions from code review

Co-authored-by: Michelle Purcell <[email protected]>

Fixes

Signed-off-by: Michal Maléř <[email protected]>

Apply suggestions from code review

Co-authored-by: Michelle Purcell <[email protected]>

Apply suggestions from code review

Co-authored-by: Michelle Purcell <[email protected]>
@MichalMaler MichalMaler force-pushed the QDOCS-99-Revamping-the-JPA-section branch from 299f022 to bc7c502 Compare February 20, 2023 17:34
@sberyozkin
Copy link
Member

Thanks @MichalMaler and @michelle-purcell

@sberyozkin sberyozkin merged commit b88d236 into quarkusio:main Feb 20, 2023
@quarkus-bot quarkus-bot bot added this to the 3.0 - main milestone Feb 20, 2023
@MichalMaler MichalMaler deleted the QDOCS-99-Revamping-the-JPA-section branch February 20, 2023 19:39
michelle-purcell
michelle-purcell reviewed Feb 28, 2023
View reviewed changes
docs/src/main/asciidoc/security-jpa-concept.adoc
The following JPA entity specification demonstrates how users' information needs to be stored in a JPA entity and properly mapped so that Quarkus can retrieve this information from a database.


* The `@UserDefinition` annotation must be present on a JPA entity, regardless of whether link:https://quarkus.io/guides/hibernate-orm-panache[simplified Hibernate ORM with Panache] is used or not.
Copy link
Contributor

Choose a reason for hiding this comment

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

@MichalMaler - Sorry just noticed this now while verifying single-sourced content on downstream docs. Should this be an xref? Thinking if not an xref, then later it might present a version mismatch as the link always points to the latest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michelle-purcell - No worries! :) Yeah, I remember this one. I didn't want to cause a broken link when downstream since this file was not diataxed, and was not on the list of the files we were originally downstreaming. If this is no issue at all, I can replace it in no time.

@gsmet
Copy link
Member

gsmet commented Feb 28, 2023

I'm unmarking all the doc PRs for backports. They are all part of massive changes that shouldn't be backported. Please don't mark for backport massive structural changes to the doc.

@michelle-purcell
Copy link
Contributor

michelle-purcell commented Feb 28, 2023 •
edited
Loading

@gsmet - OK. Thanks. Sorry for the churn.

@inoxx03 / @sberyozkin : We need to find another way to get the recent 2.16 relevant enhancements published to
image.
This is likely to involve a large PR that needs SME review. What do you think? Should we do this?

@gsmet
Copy link
Member

gsmet commented Feb 28, 2023

We need to find another way to get the recent 2.16 relevant enhancements published to

Could you explain why you would want to do that?
Because I don't think it's time well invested: we don't do large changes to the doc of a given minor once it is published.
We never did that and I don't see why it would be necessary to have these changes in 2.16. Unless I'm missing something, in that case, please correct me as it will probably happen a lot in the future.

Chances are that if the PR is very large (which is extremely probable given the PRs I have seen), I won't accept it for a micro anyway.

@michelle-purcell
Copy link
Contributor

@gsmet - Because when Quarkus customers click "read the guides" or any doc portal link, they see an older doc state.
image

They don't see content improvements that folks have done since mid-January, which is a pity.

Most of those content improvements apply also to 2.16 and are being used to source the downstream version of the equivalent guide, which is currently in the 2.x doc project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michelle-purcell michelle-purcell michelle-purcell approved these changes

@sberyozkin sberyozkin sberyozkin approved these changes

@jmartisk jmartisk Awaiting requested review from jmartisk

Assignees
No one assigned
Labels
area/documentation
Projects
None yet
Milestone
3.0.0.Alpha5
Development

Successfully merging this pull request may close these issues.
4 participants
@MichalMaler @sberyozkin @gsmet @michelle-purcell