-
Notifications
You must be signed in to change notification settings - Fork 568
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
Doc for @AddConfigBlock #8807 #8825
Conversation
Signed-off-by: Jorge Bescos Gascon <[email protected]>
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.
A few changes, please.
| `@AddConfigBlock(type = "properties", value = """ + | ||
some.key1=some.value1 + | ||
some.key2=some.value2 + | ||
""")` | Defines a new configuration (either on class level, or method level) by adding a single configuration key/value. |
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.
Please:
-
Change line 110: "Define a new" to "Define additional"
-
Change line 110: "a single configuration key/value." to "one or more configuration key/value pairs."
-
Change line 106: "Define a new" to "Define additional"
I know line 106 is not part of your changes in this PR but the original "Define a new" could be interpreted to mean "replace" which is not what it does.
-
Add code illustrating
@AddConfigBlock
to the snippet referenced immediately after the table.
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.
Thank you for the review. It is done now.
Signed-off-by: Jorge Bescos Gascon <[email protected]>
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, one more small change request.
@@ -92,6 +93,12 @@ void testConfiguredGreeting(WebTarget webTarget) { | |||
validate(webTarget, "/greet", "Unite World!"); | |||
} | |||
|
|||
@Test | |||
@AddConfigBlock("app.greeting=Unite") |
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 example should illustrate what we can do with this annotation that we cannot do with the other...declare multiple settings as in the text you added to the doc.
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 problem is that the helidon/examples/quickstarts/helidon-quickstart-mp
only uses one config property, so there is no point to add more.
I cannot create new config properties to inject in the test as a field, because of:
java.lang.RuntimeException: When a class is annotated with @HelidonTest(resetPerTest=true), injection into fields or constructor is not supported, as each test method uses a different CDI container. Field private java.lang.String io.helidon.microprofile.tests.testing.junit5.TestAddConfigBlockProperties.value1 is annotated with @Inject
Basically I need to create a new Snippet to test something like:
@HelidonTest
@AddConfigBlock("""
some.key1=some.value1
some.key2=some.value2
""")
class TestAddConfigBlockProperties {
@Inject
@ConfigProperty(name = "some.key1")
private String value1;
@Inject
@ConfigProperty(name = "some.key2")
private String value2;
@Test
void testValue() {
assertThat(value1, is("some.value1"));
assertThat(value2, is("some.value2"));
}
}
Do you agree with 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.
I have pushed the change. Just let me know whether you are fine with it.
Signed-off-by: Jorge Bescos Gascon <[email protected]>
Description
#8807
Documentation
Included in this PR