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

Update native configuration file best practices #38186

Merged

Conversation

galderz
Copy link
Member

@galderz galderz commented Jan 15, 2024

Placing custom configuration files in META-INF/native-image/<group-id>/<artifact-id> is the best way. It doesn't get in the way of what Quarkus generates, works out of the box without additional configuration and does not produce any native build warnings. Alternative methods have been removed to avoid confusion.

Copy link

github-actions bot commented Jan 15, 2024

🙈 The PR is closed and the preview is expired.

@geoand
Copy link
Contributor

geoand commented Jan 15, 2024

Is this related to #38118?

@zakkak
Copy link
Contributor

zakkak commented Jan 15, 2024

Is this related to #38118?

Yes, IMO we need a combination of these two PRs.

@galderz
Copy link
Member Author

galderz commented Jan 18, 2024

Is this related to #38118?

No it's not. This PR is the result of my own experiments.

@zakkak
Copy link
Contributor

zakkak commented Jan 18, 2024

This PR is the result of my own experiments.

@galderz sure, I think what @gsmet @geoand wanted to highlight here is that both PRs are touching the same issue. And IMO they should be combined to give the best result.

Update: Ooops wrong mention :)

@geoand
Copy link
Contributor

geoand commented Jan 18, 2024

@zakkak exactly

@galderz
Copy link
Member Author

galderz commented Jan 24, 2024

Hmm, looking at #38118, the changes there are minimal and still suggest using an experimental flag that you get a warning with (ResourceConfigurationFiles et al). So I favour my changes instead of #38118.

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

As mentioned in #38118 (review)

@galderz it looks like @dliubars updated #38118 and it now only includes the following changes (i.e. it no longer suggests using experimental options) which are currently missing from this PR:

  1. replace references to resources-config.json with resource-config.json
  2. replace references to reflection-config.json with reflect-config.json

I suggest you either:

  1. merge Rename reflection-config.json into reflect-config.json and resources-config.json into resource-config.json #38118 and rebase this PR
    or
  2. cherry-pick @dliubars commit (e911584) in your branch

* Update to promote best ways to use custom configuration files.
@galderz galderz force-pushed the topic.1501.native-configuration-documentation branch from 6dc75f3 to ff63dd6 Compare February 2, 2024 11:02
@galderz
Copy link
Member Author

galderz commented Feb 2, 2024

I merged #38118 and I've now rebased this PR with the changes there.

This comment has been minimized.

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

LGTM. Just added a suggestion that IMO makes the corresponding sentence a bit easier to read.

Copy link

quarkus-bot bot commented Feb 9, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit c575a2d.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@geoand geoand merged commit 0fbf090 into quarkusio:main Feb 9, 2024
5 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.9 - main milestone Feb 9, 2024
zakkak added a commit to zakkak/quarkus that referenced this pull request Mar 8, 2024
@galderz galderz deleted the topic.1501.native-configuration-documentation branch June 4, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants