[DRAFT] Improvement on 'Hostname' guide by adding recommended settings by scenarios/examples#20973
[DRAFT] Improvement on 'Hostname' guide by adding recommended settings by scenarios/examples#20973andre-nascimento6791 wants to merge 2 commits intokeycloak:mainfrom
Conversation
3cb56a2 to
7c09f44
Compare
There was a problem hiding this comment.
hey @andre-nascimento-rh, thanks for the PR. I added couple of suggestions. Right now I'm not really sure about the table format.
I guess we should try to make the bash commands examples in a way that a user can click on the play button in his IDE and it will automatically start the server for him (at least in development mode). I think that eventually we can get rid of the tables completely and use a simple section format with a description and an example command.
But it's just my preference after I saw the compiled result. We should definitely discuss it within the team.
7c09f44 to
d4ceee5
Compare
d4ceee5 to
e45f848
Compare
|
As you are the most experienced guy in My idea is to provide some use-case driven scenarios, starting from the simplest case and progressing to more complex cases. Please evaluate:
Your comments and suggestions are welcome. Thanks in advance. |
e45f848 to
2eba379
Compare
Pepo48
left a comment
There was a problem hiding this comment.
@andre-nascimento-rh thanks! I appreciate the progress. I added a couple of suggestions again. PTAL and let me know.
There was a problem hiding this comment.
Just nitpicks, but I think that there is still room for simplifying the structure and improving the readability.
I'm suggesting the following:
- remove the slash from every place, where it doesn't fulfil the role of an alternative - scenario/example; recommended settings/keycloak configuration; beginner/quick - all have very similar meaning in this context,
- on the other hand, let's add more precise, more detailed descriptions; use rather full sentence instead of collocations separated by commas,
- remove the bullet points - if you need, break the line instead,
- use either bold or italic text (not both at the same time).
Here's what I mean by that:
| === Scenario 01 - Beginner/Quick Test in Localhost with HTTP only | |
| * __**Description**__: | |
| ** The most basic option to run, in localhost, all default values. | |
| * __**Recommended Settings / Keycloak configuration**__: | |
| <@kc.startdev parameters="" /> | |
| * __**Expected Behavior**__: | |
| ** Both Frontend/Backend and Admin Console at: http://localhost:8080 | |
| --- | |
| === Scenario 01 - Exposing the server on localhost using HTTP | |
| .Description: | |
| Running the server on localhost using HTTP with the default settings. | |
| .Keycloak configuration: | |
| <@kc.startdev parameters="" /> | |
| All 3 endpoint categories are available at: http://localhost:8080 | |
| --- |
Regardless, I'm not sure, whether it's worth to mention this particular scenario, as it doesn't provide any additional information regarding the hostname setup (compared to others bellow). But it's up to a discussion, I used it as a reference example of how the format could be potentially improved.
There was a problem hiding this comment.
Ok. In the regard of simplifying the format, suggestions accepted and applied. Thanks.
-
remove the slash from every place, where it doesn't fulfil the role of an alternative - scenario/example; recommended settings/keycloak configuration; beginner/quick - all have very similar meaning in this context,- Done. Please review again If it is better this way.
-
on the other hand, let's add more precise, more detailed descriptions; use rather full sentence instead of collocations separated by commas- What is your suggestion of full sentence, where do you think it is not good?
-
remove the bullet points - if you need, break the line instead- Done.
-
use either bold or italic text (not both at the same time).- Done.
-
Regardless, I'm not sure, whether it's worth to mention this particular scenario, as it doesn't provide any additional information regarding the hostname setup (compared to others bellow). But it's up to a discussion, I used it as a reference example of how the format could be potentially improved.- Exactly. In my view, the most important points of this review are: Evaluate which scenarios make sense to mention in this Guide and If the
kc startcommand options are right for each scenario. And I need your help in this effort.
- Exactly. In my view, the most important points of this review are: Evaluate which scenarios make sense to mention in this Guide and If the
Thanks.
There was a problem hiding this comment.
What is your suggestion of full sentence, where do you think it is not good?
I meant it rather like a reminder that we should avoid sentence structures like the one you use initially:
The most basic option to run, in localhost, all default values.
Anyway, I think that right now the rest is ok. Thanks!
2eba379 to
a9d0891
Compare
Pepo48
left a comment
There was a problem hiding this comment.
@andre-nascimento-rh I added multiple comments/suggestions. Some of the suggestions applies for the multiple scenarios. I deliberately didn't comment every single occurrence.
Multiple walkthroughs are seemingly required anyway. We just need to improve the consistency and syntax/semantics at some places as well. Of course, in the final iterations Andy can help us with these as well. But I feel like there is still a room for an improvement that we can do on our own.
Please, if you find time, try to read through it on your own too. I feel like some of these adjustments can be easily spotted (especially the typos like the period in headings). I will do the same in the following days.
Thanks for your time and effort!
There was a problem hiding this comment.
@andre-nascimento-rh I asked about this above, in my earlier comments - is there a reason to capitalize the keywords like Proxy, Hostname, Edge etc? Unless they're the part of a heading, I would use lowercase as in normal sentence.
Also, there is lot of inconsistency in using pre-formated text (text enclosed into the backticks). Sometimes you use it, sometimes you don't. We need to be more consistent and I would suggest to not to overuse it.
Moreover, in this specific case the description consists of two sentences, which are synonymous. Please, rephrase it.
There was a problem hiding this comment.
Double-checked and changed the leaving capitalized words.
There was a problem hiding this comment.
I see that you didn't fully accepted my previous suggestion - that's ok, just please be careful about the consistency. even when in the headings. Check the existing examples in the Hostname Configuration Guide.
So I would do at least these adjustments:
| === Scenario 01 - Development Test in Localhost with HTTP only | |
| === Scenario 01 - Development test on localhost over HTTP |
There was a problem hiding this comment.
Sorry, but it exactly the opposite: I accepted almost all of your suggestions. I applied the change at the adoc file and pushed it. I understood that using capitalized words in titles there wasn't an issue for you. But now I'm changing all the titles. No problem.
There was a problem hiding this comment.
@andre-nascimento-rh I meant this specific suggestion that covered just the fist scenario. In the suggestion I deliberately changed the heading in a way that it doesn't contain uppercase letters (and also the wording is little bit different). I noticed that you didn't apply it (which is totally fine), but we need to stay consistent.
I don't even have strong preference, whether the first letter of a word in the heading should be uppercase or not (it was my natural suggestion to stay consistent with what we already have but in the end it's up to your own style), just the consistency hasn't been there yet. Therefore I reminded it. 😊
There was a problem hiding this comment.
I applied what you suggested. Are you seeing the last version?
There was a problem hiding this comment.
Explained and resolved offline.
There was a problem hiding this comment.
We are mentioning the necessity to setup TLS for HTTPS scenarios (for the production mode) right at the beginning. Is it necessary to repeat the same in the descriptions of the particular scenarios?
There was a problem hiding this comment.
Yes, I believe it is clearer.
There was a problem hiding this comment.
ok, then we should at least correct the capital letter in "Certificates".
There was a problem hiding this comment.
| Your `reverse proxy` doesn't have HTTPS/TLS termination and trust in Keycloak. And you provide access in HTTP only to Keycloak behind your `proxy` (Not recommended at all!). | |
| Reverse proxy doesn't have HTTPS/TLS termination and trust in Keycloak. The access to your Keycloak instance behind the proxy is then provided over HTTP only, which is not recommended. |
Btw, this is a good example where I would use an additional formatting to highlight "not recommended part".
There was a problem hiding this comment.
Done. Review it If that way suits your expectation.
There was a problem hiding this comment.
What I meant by this suggestion was to get rid of the bracket part (the capital letter and exclamation mark specifically).
And during the process I thought also about minimizing the usage of second-person pronouns (e.g. you) and possessive pronouns (e.g. your). But I would rather leave this opened for someone with better linguistic knowledge.
a9d0891 to
97cc469
Compare
|
@andre-nascimento-rh please do not mark the conversation as resolved and leave that to the reviewers to resolve. |
|
@andre-nascimento-rh do you think that we could break the draft PR into the smaller pieces (logically related chunks) and try to merge them one by one? Let's for example start with those about which we are confident enough. So I would say let's be conservative and take just two scenarios from this draft - 05 and 12. In my opinion both of them are providing something, which is not yet fully covered in the current version of the guide, both are HTTPS examples (I think we should predominantly demonstrate examples that are production ready) and on top of that we can take some information from the other examples (reencrypt, passthrough) and squash them under these two as well. The PR would be much smaller, easier to digest and review. Wdyt? |
|
cc: @mabartos ^^ |
97cc469 to
1e0bdbf
Compare
@Pepo48, I agree with the suggestion of breaking the draft PR into the smaller pieces. Let's go for it from the next push.
Even I'm not a big fan of this approach, still there are people interested in making Keycloak responding only over HTTP, and some of them stuck in how to achieve that. Therefore I would still keep or add some scenarios to run only over HTTP. WDYT? |
…ped scenarios/examples. Closes keycloak#20931
…ped scenarios/examples. Closes keycloak#20931
1e0bdbf to
0d36630
Compare
|
This PR is superseded by #21846. |
Considering the analysis done in Issue #20571 on each issue mentioned in the umbrella Issue #14666, it was decided that the first action to take is to improve the documentation/guides which are related to Hostname, Frontend and Admin Console and Proxy settings.
Closes #20931