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

Explain @ConfigGroup and @ConfigRoot in more details #2589

Merged
merged 1 commit into from
Jun 11, 2019

Conversation

sarxos
Copy link
Contributor

@sarxos sarxos commented May 24, 2019

Hello.

This commit is in regards to our discussion on Zulip. See https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/Extensions for more details.

My concern was in regards to the following statement from the "2.4.6. Configuration Example" in Extensions tutorial here https://quarkus.io/guides/extension-authors-guide.

  1. The @configroot annotation indicates that this object is a configuration root group, whose property names will have a parent only of quarkus.. In this case the properties within the group will begin with quarkus.log.*.

Where the code sample looks like this:

@ConfigRoot(phase = ConfigPhase.RUN_TIME)
public class LogConfiguration {

    // ...

    /**
     * Configuration properties for the logging file handler.
     */
    FileConfig file;
}

And where point 2 states:

... indicates that this object is a configuration root group, whose property names will have a parent only of quarkus.. In this case the properties within the group will begin with quarkus.log.*.

But where is this indication? How this class is linked to quarkus.log.*? Is there some declaration missing?


This PR is to (hopefully) make this paragraph more clear and to document details you guys provided to me in the Zulip chat, since these details are not explained anywhere else.

Copy link
Member

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

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

Looks mostly OK with a couple small issues.

docs/src/main/asciidoc/extension-authors-guide.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/extension-authors-guide.adoc Outdated Show resolved Hide resolved
@dmlloyd
Copy link
Member

dmlloyd commented May 28, 2019

In fact, changing the default log file name is pretty significant and shouldn't be "hidden" as a minor doc change. I stupidly misread this change, it really is just a doc change but it is misleading because of the doc content.

@gsmet
Copy link
Member

gsmet commented May 29, 2019

@sarxos could you take our comments into account and push an update? Thanks!

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.

I added a couple of minor comments.

docs/src/main/asciidoc/extension-authors-guide.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/extension-authors-guide.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/extension-authors-guide.adoc Outdated Show resolved Hide resolved
@sarxos
Copy link
Contributor Author

sarxos commented May 30, 2019

@dmlloyd @gsmet Thank you for review! Before I address your comments I would like to discuss them. Especially the log matter because I changed this intentionally to make this example less confusing. Let's discuss under the comments.

@sarxos
Copy link
Contributor Author

sarxos commented May 30, 2019

@gsmet @dmlloyd I hope I addressed all of your comments. Can we do next code review iteration and merge this PR if there are no more issues here?

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.

I added a few comments. Could you check them out?

docs/src/main/asciidoc/extension-authors-guide.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/extension-authors-guide.adoc Outdated Show resolved Hide resolved
@sarxos
Copy link
Contributor Author

sarxos commented Jun 5, 2019

@gsmet I addressed some of your comments but I would like to discuss the remaining one. For a readability sake I would like to avoid having any mention of quarkus.log in this example in terms other than a log namespace. This is the core reason why I changed this asciidoc paragraph - because it was confusing as hell :)

gsmet
gsmet previously requested changes Jun 5, 2019
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.

I added a couple more comments. Once you are done with them, I think it will be OK.

Could you squash everything in one commit at the end?

Thanks!

docs/src/main/asciidoc/extension-authors-guide.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/extension-authors-guide.adoc Outdated Show resolved Hide resolved
@sarxos
Copy link
Contributor Author

sarxos commented Jun 5, 2019

@gsmet I replaced DEFAULT_LOG_FILE_NAME with the application.log. Please let me know if this is ok and then I will squash these commits.

@cescoffier
Copy link
Member

Waiting for CI.

@cescoffier cescoffier added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 5, 2019
@cescoffier
Copy link
Member

@gsmet @dmlloyd are you ok with the latest changes?

@dmlloyd
Copy link
Member

dmlloyd commented Jun 5, 2019

To quote @gsmet:

We need this example to compile. We can't give a doc example that doesn't compile. So if you don't want to use the constant, just use quarkus.log as it was before. It is just an example so it doesn't need to be totally similar to the code we have in Quarkus.

And round and round we go. Using logging for the example was clearly a mistake!

@sarxos
Copy link
Contributor Author

sarxos commented Jun 6, 2019

Can we just leave it as it is now? In its current form it's sufficient to serve as an example, IMO.

@cescoffier cescoffier added this to the 0.17.0 milestone Jun 6, 2019
@cescoffier cescoffier removed coding-in-progress triage/waiting-for-ci Ready to merge when CI successfully finishes labels Jun 6, 2019
@cescoffier
Copy link
Member

@sarxos yes we keep it, the idea of using log was taken a long time ago.

@cescoffier cescoffier requested review from gsmet and dmlloyd June 6, 2019 16:12
Copy link
Member

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

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

I have no opinion one way or another about the change.

@dmlloyd dmlloyd dismissed their stale review June 6, 2019 17:38

Removing old review

@sarxos
Copy link
Contributor Author

sarxos commented Jun 10, 2019

Hi @cescoffier,

What should I do with this pull request? Are we merging it or shall I close it as unnecessary? IMO it should be merged since existing example in its current form, together with related explanation, is really confusing. The changes I did are to clarify it, at least a little bit.

Address code review comments in regards to log file name
@sarxos
Copy link
Contributor Author

sarxos commented Jun 10, 2019

Commits are now squashed.

@cescoffier cescoffier dismissed gsmet’s stale review June 11, 2019 06:25

The comments have been handled.

@cescoffier
Copy link
Member

I think we are good to go. This PR clearly shows an issue with our example (log) and we may need to come with something different - less controversial as an example.

@cescoffier cescoffier merged commit e76ae7c into quarkusio:master Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants