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

Clarify usage of quarkus-keycloak-admin-rest-client and quarkus-keycloak-admin-resteasy-client #45910

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

rolfedh
Copy link
Contributor

@rolfedh rolfedh commented Jan 27, 2025

Clarify usage of quarkus-keycloak-admin-rest-client and quarkus-keycloak-admin-resteasy-client.

@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation labels Jan 27, 2025
@rolfedh rolfedh requested a review from sberyozkin January 27, 2025 23:10
@rolfedh rolfedh force-pushed the update-conditionals-bis branch from 6ed5b99 to 8fd0e8f Compare January 27, 2025 23:11

This comment has been minimized.

Copy link

github-actions bot commented Jan 28, 2025

🙈 The PR is closed and the preview is expired.

@sberyozkin
Copy link
Member

Hi Rolfe, neither quarkus-keycloak-admin-rest-client nor quarkus-keycloak-admin-resteasy-client are currently supported, so I'm not sure if these exclusions are correct...

@rolfedh rolfedh force-pushed the update-conditionals-bis branch from 8fd0e8f to 7fe4bf1 Compare January 28, 2025 12:40

This comment has been minimized.

@rolfedh
Copy link
Contributor Author

rolfedh commented Jan 28, 2025

Hi Rolfe, neither quarkus-keycloak-admin-rest-client nor quarkus-keycloak-admin-resteasy-client are currently supported, so I'm not sure if these exclusions are correct...

If the Keycloak Admin Client (KAC) isn’t supported in the product, then the current conditionalization is (mostly) correct.

Using the ifndef:: directive to enclose KAC content ensures it appears in the community docs but remains hidden in the product documentation.

The only concern I see is that updating the conditional to no-quarkus-keycloak-admin-resteasy-client might make it too specific, applying only to one of the unsupported extensions. (Is this what you were referencing in your comment?)

Perhaps it would be better to retain no-quarkus-keycloak-admin-client, as it more transparently reflects its role in hiding both extensions. What are your thoughts?

@sberyozkin
Copy link
Member

@rolfedh

The only concern I see is that updating the conditional to no-quarkus-keycloak-admin-resteasy-client might make it too specific, applying only to one of the unsupported extensions. (Is this what you were referencing in your comment?)

Yes,

Perhaps it would be better to retain no-quarkus-keycloak-admin-client, as it more transparently reflects its role in hiding both extensions. What are your thoughts?

If it can hide both extensions then it would indeed be the right solution

Thanks

@rolfedh
Copy link
Contributor Author

rolfedh commented Jan 28, 2025

If it can hide both extensions then it would indeed be the right solution

Yes, it does. I’ve verified that this setup correctly handles all instances of these extensions in both the upstream and downstream documentation.

This PR will focus solely on elaborating on the existing note, not on updating the conditionals. Additionally, I’ll include a note in the downstream attributes to clarify the dual role of no-quarkus-keycloak-admin-client and its relationship to both quarkus-keycloak-admin-rest-client and quarkus-keycloak-admin-resteasy-client.

@rolfedh rolfedh changed the title Update conditionals with current extension name Clarify usage of quarkus-keycloak-admin-rest-client and quarkus-keycloak-admin-resteasy-client Jan 28, 2025
@rolfedh rolfedh force-pushed the update-conditionals-bis branch from 7fe4bf1 to 1008481 Compare January 28, 2025 14:09
@rolfedh rolfedh requested a review from sberyozkin January 28, 2025 14:09

This comment has been minimized.

@rolfedh rolfedh force-pushed the update-conditionals-bis branch from 1008481 to ea48e98 Compare January 28, 2025 17:09
@rolfedh rolfedh requested review from michalvavrik and gsmet January 28, 2025 17:11
@rolfedh rolfedh force-pushed the update-conditionals-bis branch from ea48e98 to e66178e Compare January 28, 2025 17:11

This comment has been minimized.

Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

thanks @rolfedh

@rolfedh rolfedh force-pushed the update-conditionals-bis branch from e66178e to 4a394fc Compare January 28, 2025 21:48
@rolfedh rolfedh force-pushed the update-conditionals-bis branch from 4a394fc to fa620fa Compare January 28, 2025 21:49
@rolfedh
Copy link
Contributor Author

rolfedh commented Jan 28, 2025

I had to make another push to restore the cross reference to the Keycloak guide on line 244. We have approvals from @sberyozkin and @michalvavrik. I think this PR is only needs final review and merge from @gsmet. Thank you all for your contributions and reviews!

Copy link

quarkus-bot bot commented Jan 28, 2025

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit fa620fa.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@sberyozkin
Copy link
Member

Guillaume, @gsmet, please double check if you can get a few minutes as Rolfe would like you to review, thanks

@gsmet gsmet merged commit 4c2855c into quarkusio:main Jan 29, 2025
5 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.19 - main milestone Jan 29, 2025
@gsmet
Copy link
Member

gsmet commented Jan 29, 2025

Yes, it was on my list :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docstyle issues related for manual docstyle review area/documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants