Skip to content

Conversation

@jbonofre
Copy link
Member

@jbonofre jbonofre commented Feb 20, 2025

In order to give a better "visibility" about Polaris configuration properties to end user, this PR add $PWD/config/application.properties to the Polaris server distribution.

@dennishuo it was my proposal when you said you are struggling to find "configuration".

@adutra
Copy link
Contributor

adutra commented Feb 20, 2025

FYI, an external application.properties file under $PWD/config is supported out of the box. This is the approach taken by the Helm chart (i.e. generate a configmap and mount it under $PWD/config/application.properties).

What is the use case for yet another external configuration file?

@jbonofre
Copy link
Member Author

@adutra I was hesitating by using $PWD/config/application.properties, mostly for the non Quarkus users. But you are right, it's a better approach. I will update the PR according, updating gradle to "pre-populate" config/application.properties with polaris.conf content.

Thanks !

@adutra
Copy link
Contributor

adutra commented Feb 20, 2025

I think using the default external location is more prudent indeed, we don't need to get into the business of deciding what to do if two external files are present :-)

@jbonofre
Copy link
Member Author

I think using the default external location is more prudent indeed, we don't need to get into the business of deciding what to do if two external files are present :-)

Yeah, agree. My bad 😄

@jbonofre jbonofre force-pushed the polaris-external-config branch from fe4dd8a to 849a7ea Compare February 20, 2025 17:40
@jbonofre jbonofre changed the title Add PolarisConfigSource to optional load a "external" polaris.conf file for the end user Add $PWD/config/application.properties configuration in the distribution Feb 20, 2025
@jbonofre
Copy link
Member Author

@adutra I updated the PR according to your PR.

… to be "visible" to end user in the distribution
@jbonofre jbonofre force-pushed the polaris-external-config branch from 849a7ea to bc58d74 Compare February 20, 2025 18:04
# under the License.
#

io.smallrye.config.LoggingConfigSourceInterceptor No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should not have been deleted, only reverted.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right, let me fix that.

# under the License.
#

# This is the main and classic Apache Polaris configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this file is loaded related to the current directory in runtime, not relative to the location of the jar... So it is fine to include it in the distribution, but I believe there may still be odd cases when users start Polaris from some arbitrary location and this file is not found.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. For now I deal only about distribution. Majority of users will user docker or distribution.

@jbonofre
Copy link
Member Author

jbonofre commented Mar 4, 2025

@dennishuo @eric-maynard @adutra @dimas-b what do you think about this PR ? Do you think it could be helpful for our users (to easily find and see all classic configuration properties) ?
If not, I'm happy to close this PR.

Thoughts ?

@eric-maynard
Copy link
Contributor

I'm a bit confused -- quarkus/server/src/main/distribution/config/application.properties doesn't look like the same thing as $PWD to me. Does this really do what the description says?

this PR add $PWD/config/application.properties to the Polaris server distribution.

@eric-maynard
Copy link
Contributor

Could we just allow for a top level (i.e. .../polaris/) config file which then gets copied to the requisite Quarkus location at build time?

@jbonofre
Copy link
Member Author

jbonofre commented Mar 4, 2025

@eric-maynard I like your idea of having a config.properties in the root folder that we copy at build time. That's actually even more a good way to provide "user visible".

Let me update this PR this way.

About $PWD, it's from a distribution perspective. I agree that could be confusing.

@dimas-b
Copy link
Contributor

dimas-b commented Mar 4, 2025

From my POV, I'd prefer educating users on how to benefit from the multitude of ways to configure Polaris (which come from the Quarkus infra). I believe it will be an advantage to all users in the long run, even though there is a learning curve.

I personally think that combining multiple config sources at build time will complicate the build unnecessarily. This looks similar to users (currently) having to provide extra build parameters for PostgreSQL, which makes every build different at the binary level.

Ideally, I think no rebuild should be necessary to switch between database backends or for any other runtime config changes (only a restart with different config values should do it).

A few properties that do require re-compilation are very low-level and changing them is something that normally needs a PR, IMHO :)

@jbonofre
Copy link
Member Author

jbonofre commented Mar 5, 2025

Fair enough, thanks everyone for your feedback.

@jbonofre jbonofre closed this Mar 5, 2025
@github-project-automation github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board Mar 5, 2025
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