-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Docs - Add missing steps to Basic authentication how-to #37121
Merged
rsvoboda
merged 1 commit into
quarkusio:main
from
michelle-purcell:doc-qe-fix-security-ba
Nov 16, 2023
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry Michelle but I'm not sure we need to highlight this property at all, I don't recall ever using it for tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is enabled by default I guess, so may be below it works but if if it is enabled by default then it is 1 extra property to configure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I use it here https://github.com/quarkusio/quarkus/blob/main/integration-tests/csrf-reactive/src/main/resources/application.properties#L6 :-) but I'd not highlight in isolation, below it is fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the noise, if you'd like to highlight then it is fine, but IMHO it it should all be again done below, otherwise it reads as if it can be used outside of the testing, which it technically can be but we don't recommend it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default is false - see https://quarkus.io/guides/all-config#quarkus-elytron-security-properties-file_quarkus.security.users.embedded.enabled
I think it's fine to have the property just in the example bellow. IMHO no need to highlight this property in this separate part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to @rsvoboda's suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sberyozkin , @rsvoboda , @jsmrcka
Thank you for your input and reviews on this bit too.
I think we have to keep in mind that the person reading this introductory-level how-to topic is likely to be new to Quarkus and is therefore likely 'playing' in a test environment. So while we don't recommend it for production, +1 to keeping it in and being consistent elsewhere.
I made some adjustments to the procedure to highlight that some configuration steps are purely for testing purposes. How does this look now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rsvoboda & @jsmrcka - I also propose that we diverge in the product version a little and either:
( @tqvarnst @inoxx03 & @[email protected] FYI)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prereq section should be sufficient, otherwise we would need to drop all 3 mentioned extensions :)