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

config yaml guide updates #32869

Closed
wants to merge 2 commits into from
Closed

Conversation

rolfedh
Copy link
Contributor

@rolfedh rolfedh commented Apr 24, 2023

Fixes #32890

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 24, 2023

Thanks for your pull request!

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

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@github-actions
Copy link

github-actions bot commented Apr 24, 2023

🙈 The PR is closed and the preview is expired.

@rolfedh rolfedh force-pushed the document-yaml-config branch 4 times, most recently from 08aabf2 to d5a8dca Compare April 27, 2023 21:04
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Thanks, I added a couple of comments but I'd like @radcortez to have a look too.

@rolfedh rolfedh force-pushed the document-yaml-config branch 4 times, most recently from df3ce41 to fcf093c Compare May 9, 2023 16:39
@rolfedh
Copy link
Contributor Author

rolfedh commented May 9, 2023

Thanks for your review and comments, @gsmet & @radcortez. I've made some updates based on your suggestions and left a few suggestions from @radcortez open so I can ask for clarification when we meet tomorrow.

@rolfedh rolfedh force-pushed the document-yaml-config branch 5 times, most recently from 293e4e8 to f97bb6c Compare May 13, 2023 19:33
@rolfedh rolfedh closed this May 13, 2023
@rolfedh rolfedh force-pushed the document-yaml-config branch from f97bb6c to ba59762 Compare May 13, 2023 20:39
@rolfedh rolfedh force-pushed the document-yaml-config branch 2 times, most recently from 769090e to 283493e Compare May 18, 2023 17:47
@rolfedh rolfedh force-pushed the document-yaml-config branch 2 times, most recently from 634beb8 to 9c70135 Compare May 30, 2023 20:42
@rolfedh rolfedh force-pushed the document-yaml-config branch 2 times, most recently from 626ac8d to 8847b17 Compare July 4, 2023 20:13
Copy link
Member

@radcortez radcortez left a comment

Choose a reason for hiding this comment

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

I left a couple of additional comments. Once those are resolved, I think it should be good to go. Thanks!

@rolfedh rolfedh force-pushed the document-yaml-config branch from 8847b17 to e47de51 Compare July 5, 2023 00:26
@radcortez
Copy link
Member

I think @gsmet may still have some concerns.

@rolfedh
Copy link
Contributor Author

rolfedh commented Jul 5, 2023

Hi @gsmet. I welcome your thoughts and comments. I'm getting ready to put it through our official QE process and want to make sure the content is settled before then.

@rolfedh rolfedh force-pushed the document-yaml-config branch from e47de51 to 994ff8c Compare July 5, 2023 10:21
@rolfedh rolfedh requested a review from gsmet July 5, 2023 14:13
@rolfedh rolfedh force-pushed the document-yaml-config branch from 994ff8c to 4f8ad84 Compare July 5, 2023 20:16
@rolfedh
Copy link
Contributor Author

rolfedh commented Jul 5, 2023

@gsmet I'm not sure whether to wait for your input. I've gone ahead and requested QE review via https://issues.redhat.com/browse/QUARKUS-3243. Please let me know if you would like me postpone.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Review updated to clarify my thoughts: I'm unhappy with the current result.

It is less precise, sometimes incorrect, and far more verbose than it used to be.

Developers don't have a lot of time when they read documentation. If we are making every small detail extremely verbose, they won't read our documentation.
Also we really need to avoid adding very vague sentences that are not actually useful.

I tried to point out some of the issues but really it's a general impression about this update.

docs/src/main/asciidoc/config-yaml.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/config-yaml.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/config-yaml.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/config-yaml.adoc Show resolved Hide resolved

Remove the `src/main/resources/application.properties` and create a `src/main/resources/application.yaml` file.
In the YAML configuration file, you can define a nested element inside an existing element to set configuration properties for your {project-name} application.
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this sentence and what it brings to the plate?

I mean, this is the basics of YAML so it's not an extra feature or something that should be in a separate paragraph?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the "nested" term. This section is very basic but might help non-experts by showing an example configuration file. WDYT?

== Expressions
// meta:include::modules/proc_setting-custom-configuration-profiles-with-yaml.adoc[leveloffset=+2]
[id="proc_setting-custom-configuration-profiles-with-yaml_{context}"]
== Setting custom configuration profiles with YAML
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the value of this paragraph compared to the previous one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Removed this section.

// DELETE meta:include::modules/con_external-application-yaml-file-for-configuring-properties-at-runtime.adoc[leveloffset=+1]
// CREATE meta:include::modules/proc_configure-properties-at-run-time-external-yaml.adoc[leveloffset=+1]
[id="proc_configure-properties-at-run-time-external-yaml_{context}"]
== Configure properties at run time
Copy link
Member

Choose a reason for hiding this comment

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

Don't we write it runtime? At least that's how we used to write it everywhere? Did it change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IBM Style states:

run time, runtime noun
- Write as two words to refer to the time at which an application runs (for example, “At run time, you can…”).
- Write as one word to refer to the environment in which an application runs (for example, “The Java runtime is
controlled by…”).

However, if you feel strongly about it, we can use runtime. Most users are probably fine with it as a single word. Just let me know.

Comment on lines +399 to +407
+
. To compile your {project-name} application in development mode, enter the following command from the project directory:
+
.Compile your {project-name} application
[source,bash]
----
./mvnw quarkus:dev
----
+
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure of the value of this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally, this type of information is useful for non-expert users such as students. Red Hat generally gets complaints from newer users that our documentation assumes too much expertise or prior knowledge.


== Configuration key conflicts
Structured formats such as YAML only support a subset of the possible configuration namespace.
The following procedure shows how to resolve a conflict between two configuration properties, `quarkus.http.cors` and `quarkus.http.cors.methods`, where one property is the prefix of another.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the rules and maybe this needs to be discussed at a higher level but we are adding a ton of procedures/prerequisites and this adds a lot of noise when you are trying to find some information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please feel free to strike any information that doesn't add value for our customers.
In future projects, rather than trying to merge the community and product content, I'll adopt an "additive" process. That is, I'll ask the SME to identify which product content they want to migrate to the community docs first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or, if you feel like that's too much trouble, I can toss out this PR and we start over with the proposed "additive" process instead.

docs/src/main/asciidoc/config-yaml.adoc Outdated Show resolved Hide resolved
@rolfedh
Copy link
Contributor Author

rolfedh commented Jul 6, 2023

Thanks again for these additional comments. I'm making further updates and replying to some of your questions.

Copy link
Contributor Author

@rolfedh rolfedh left a comment

Choose a reason for hiding this comment

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

Thanks for your feedback, @gsmet. I've made updates and am open to more. Please have a look at my replies in the open conversations.

docs/src/main/asciidoc/config-yaml.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/config-yaml.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/config-yaml.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/config-yaml.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/config-yaml.adoc Show resolved Hide resolved
// DELETE meta:include::modules/con_external-application-yaml-file-for-configuring-properties-at-runtime.adoc[leveloffset=+1]
// CREATE meta:include::modules/proc_configure-properties-at-run-time-external-yaml.adoc[leveloffset=+1]
[id="proc_configure-properties-at-run-time-external-yaml_{context}"]
== Configure properties at run time
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IBM Style states:

run time, runtime noun
- Write as two words to refer to the time at which an application runs (for example, “At run time, you can…”).
- Write as one word to refer to the environment in which an application runs (for example, “The Java runtime is
controlled by…”).

However, if you feel strongly about it, we can use runtime. Most users are probably fine with it as a single word. Just let me know.


== Configuration key conflicts
Structured formats such as YAML only support a subset of the possible configuration namespace.
The following procedure shows how to resolve a conflict between two configuration properties, `quarkus.http.cors` and `quarkus.http.cors.methods`, where one property is the prefix of another.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please feel free to strike any information that doesn't add value for our customers.
In future projects, rather than trying to merge the community and product content, I'll adopt an "additive" process. That is, I'll ask the SME to identify which product content they want to migrate to the community docs first.

Comment on lines +399 to +407
+
. To compile your {project-name} application in development mode, enter the following command from the project directory:
+
.Compile your {project-name} application
[source,bash]
----
./mvnw quarkus:dev
----
+
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally, this type of information is useful for non-expert users such as students. Red Hat generally gets complaints from newer users that our documentation assumes too much expertise or prior knowledge.

== Expressions
// meta:include::modules/proc_setting-custom-configuration-profiles-with-yaml.adoc[leveloffset=+2]
[id="proc_setting-custom-configuration-profiles-with-yaml_{context}"]
== Setting custom configuration profiles with YAML
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Removed this section.


== Configuration key conflicts
Structured formats such as YAML only support a subset of the possible configuration namespace.
The following procedure shows how to resolve a conflict between two configuration properties, `quarkus.http.cors` and `quarkus.http.cors.methods`, where one property is the prefix of another.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or, if you feel like that's too much trouble, I can toss out this PR and we start over with the proposed "additive" process instead.

@rolfedh
Copy link
Contributor Author

rolfedh commented Jul 13, 2023

Closing this PR to stop adding to sunken costs. Starting over with updated priorities and methods.

@rolfedh rolfedh closed this Jul 13, 2023
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a "Configuring your Quarkus applications by using a YAML file" guide
5 participants