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

Improve the Basic Authentication section #26697

Conversation

MichalMaler
Copy link
Contributor

@MichalMaler MichalMaler commented Jul 13, 2022

Improve the Basic Authentication section

Adding intro for the BA section and also the information about how to configure it.

Close:
https://issues.redhat.com/browse/QDOCS-27
https://issues.redhat.com/browse/QDOCS-28

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 13, 2022

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should count at least 2 words to describe the change properly

This message is automatically generated by a bot.

@MichalMaler MichalMaler self-assigned this Jul 13, 2022
@MichalMaler MichalMaler force-pushed the QDOCS-27-28-Improoving-the-Based-Auth-section branch from d6271ce to f603cc3 Compare July 13, 2022 07:28
@gsmet
Copy link
Member

gsmet commented Jul 13, 2022

Hi @MichalMaler ,

A few comments on the form of the pull request:

  • Please make the title of the PR a sentence similar to Improve the Basic Authentication section - it's important as they are included in our community release notes
  • And then include the issue numbers in the description as you did
  • And same for the commit comment

If you form your commit comment like:

Improve the Basic Authentication section

Close:
https://issues.redhat.com/browse/QDOCS-27
https://issues.redhat.com/browse/QDOCS-28

(note the blank line, it's important)

Then GitHub will automatically fill the fields appropriately when creating the pull request if you have only one commit and your work will be easier.

/cc @ebullient @quarkusio/documentation for general awareness.

@MichalMaler MichalMaler changed the title QDOCS-27-28-Improoving-the-Based-Auth-section Improve the Basic Authentication section Jul 13, 2022

In the context of an HTTP request, basic access authentication is a method for an HTTP user agent (such as a Web browser) to provide a user name and password when making a request. In basic HTTP authentication, a request contains a header field in the form of _Authorization: Basic_ _<credentials>_, where credentials are the Base64 encoding of a user ID and password joined by a colon.

====
Copy link
Member

Choose a reason for hiding this comment

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

We never used this construct without an admonition type (e.g. [NOTE]) so we need to check how it renders on the website.

/cc @ebullient

Copy link
Member

Choose a reason for hiding this comment

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

@gsmet Do you mean this one, _Authorization: Basic_ _<credentials>_ ?
I'd just go for a simple highlighting like Authorization: Basic <credentials>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sberyozkin I think he was talking about the ==== box I used to highlight the example.
The underscores mean a plain italic.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, what @MichalMaler said.

Copy link
Member

Choose a reason for hiding this comment

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

Authorization: Basic <credentials> should be inline code, rather than italics, I believe.

Without the type (e.g. NOTE), it is a simple admonition that should render as a plain callout box, but we will have to see what the web CSS does with it.

@MichalMaler MichalMaler force-pushed the QDOCS-27-28-Improoving-the-Based-Auth-section branch from f603cc3 to a97cc7c Compare July 13, 2022 07:46
@MichalMaler
Copy link
Contributor Author

@gsmet Hello there. Thank you for the feedback. I applied what I could. Let see how it looks on the portal and if Erin will like it.
BTW, I tried to used the ./mvnw -f docs clean install command to build it locally, but I run into some 999-SNAPSHOT error.
Any idea how to make it work, please?

Cheers!

@MichalMaler MichalMaler force-pushed the QDOCS-27-28-Improoving-the-Based-Auth-section branch 2 times, most recently from 1f7db44 to 14f83ec Compare July 13, 2022 08:25
@gsmet
Copy link
Member

gsmet commented Jul 13, 2022

@MichalMaler yeah, you need to do a full build first. Do a ./mvnw -DquicklyDocs then wait for a long time and you'll be all set.

Then you can iterate on the docs module as you wanted to do it without doing a full build every time. You might have to do it from time to time when we add a new module but it doesn't happen that often these days so you should be pretty much OK.

@sberyozkin sberyozkin self-requested a review July 13, 2022 08:30
Copy link
Contributor

@hmanwani-rh hmanwani-rh left a comment

Choose a reason for hiding this comment

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

@MichalMaler Thank you for creating this PR. The changes look great.
I'm adding a few suggestions to the changes to align with the style guide, please take a look and let me know if you've any questions.

@MichalMaler MichalMaler force-pushed the QDOCS-27-28-Improoving-the-Based-Auth-section branch from 14f83ec to 55ba072 Compare July 13, 2022 10:17
@MichalMaler MichalMaler force-pushed the QDOCS-27-28-Improoving-the-Based-Auth-section branch from 55ba072 to 5cf1067 Compare July 13, 2022 11:13
@MichalMaler MichalMaler force-pushed the QDOCS-27-28-Improoving-the-Based-Auth-section branch from 5cf1067 to 14a528f Compare July 13, 2022 11:40
@sberyozkin
Copy link
Member

@MichalMaler @hmanwani-rh Thanks very much for this effort, LGTM, sorry if some of my suggestions look a bit pedantic. Your PR improves Basic Authentication related documentation a lot. I guess it is only the first step, eventually it will be moved into the dedicated doc(s)

Copy link
Member

@ebullient ebullient left a comment

Choose a reason for hiding this comment

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

Just to confirm, this is a first pass at revising an existing doc, right? (I think I remember this).. we're kind of in the fuzzy middle to me on this one (a cross between a how-to and an explanation... )

@sberyozkin
Copy link
Member

LGTM, I can merge shortly, my understanding is that right now the task is about getting the content right, the actual restructuring to align with new templates will follow up

@sberyozkin
Copy link
Member

@ebullient right, just saw your comment, yes, that is my understanding too. The fist pass is about revising the current content for different security documents, preparing them really for the next phase and then in the next phase it will be a matter of moving the chunks of content around to align with the new model

@sberyozkin sberyozkin merged commit 0c76838 into quarkusio:main Jul 13, 2022
@quarkus-bot quarkus-bot bot added this to the 2.12 - main milestone Jul 13, 2022
@MichalMaler MichalMaler deleted the QDOCS-27-28-Improoving-the-Based-Auth-section branch July 14, 2022 09:27
@gsmet gsmet modified the milestones: 2.12 - main, 2.11.0.Final Jul 19, 2022
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.

5 participants