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

FISH-9198 Add Microprofile Config TOML Support #6988

Merged
merged 2 commits into from
Oct 14, 2024

Conversation

kalinchan
Copy link
Member

@kalinchan kalinchan commented Oct 1, 2024

Description

Resolves #6822

This feature introduces changes to the Admin Console and new AsAdmin Commands.

By default the ordinal for TOML is 95.

The properties will only be updated when first initialised or if the last modified time has been changed.

When the TOML file contains a nested array you can retrieve the value by specifying the index e.g. [1]

Admin Console

(Configurations -> server-config -> Microprofile -> Config -> TOML)
image

AsAdmin Commands

  • get-toml-config-source-configuration: Returns the path to the TOML file and the maximum recursion depth (to prevent potential stackoverflow)
  • set-toml-config-source-configuration: path parameter to specify location of toml file, can be relative to current domain. depth parameter to specify the depth of recursion.

Important Info

Blockers

n/a

Testing

New tests

none

Testing Performed

Specified a TOML file with various different tables and arrays and deployed a simple application that prints out all properties, the ordinal and specific values.

Testing Environment

Apache Maven 3.9.8 (36645f6c9b5079805ea5009217e36f2cffd34256)
Maven home: /Users/kalinchan/Library/apache-maven-3.9.8
Java version: 11.0.23, vendor: Azul Systems, Inc., runtime: /Users/kalinchan/.sdkman/candidates/java/11.0.23-zulu/zulu-11.jdk/Contents/Home
Default locale: en_GB, platform encoding: UTF-8
OS name: "mac os x", version: "14.6.1", arch: "aarch64", family: "mac"

Documentation

to come...

Notes for Reviewers

Had to implement the ordinal delegation for configsourceproxy.

Removed default recursion depth as it does not appear in admin console or when running admin command.

@kalinchan kalinchan force-pushed the FISH-9198 branch 3 times, most recently from d8327e7 to 9a393a7 Compare October 1, 2024 17:47
@luiseufrasio luiseufrasio self-requested a review October 4, 2024 10:30
Copy link
Contributor

@luiseufrasio luiseufrasio left a comment

Choose a reason for hiding this comment

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

When the file does not exists on Windows it returns:

asadmin set-toml-configuration --path C:\config.tom --depth 10

Picked up JAVA_TOOL_OPTIONS: -Duser.language=en -Duser.country=US -Dfile.encoding=UTF-8
remote failure: java.nio.file.InvalidPathException: Illegal char <:> at index 105: C:\Users\luise\git\Payara\appserver\distributions\payara\target\stage\payara6\glassfish\domains\domain1\C:\config.tom
Illegal char <:> at index 105: C:\Users\luise\git\Payara\appserver\distributions\payara\target\stage\payara6\glassfish\domains\domain1\C:\config.tom
Command set-toml-configuration failed.

core/pom.xml Outdated Show resolved Hide resolved
Copy link
Member

@Pandrex247 Pandrex247 left a comment

Choose a reason for hiding this comment

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

I would also question whether the TOML config source should exist under the nucleus package - would it not be better homed alongside the other extensions?

Copy link
Contributor

@luiseufrasio luiseufrasio left a comment

Choose a reason for hiding this comment

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

LGTM!

@Pandrex247 Pandrex247 added the PR: DO NOT MERGE Don't merge PR until further notice label Oct 9, 2024
@poikilotherm
Copy link
Contributor

poikilotherm commented Oct 9, 2024

Potentially stupid question, late to the party. Instead of using TOMLJ and adding another dependency, would it help to use Jackson Dataformat TOML, which is already around as a lib?

@Pandrex247
Copy link
Member

Potentially stupid question, late to the party. Instead of using TOMLJ and adding another dependency, would it help to use Jackson Dataformat TOML, which is already around as a lib?

My memory may be failing me, but I don't recall us discussing this library in particular, and it doesn't seem to be indexed highly in Google - it's entirely possible we may have simply overlooked the existence of this library.

Thanks for the suggestion @poikilotherm: we will investigate if it's suitable. We already use Jackson for XML and YAML parsing in the server so yes, all things being equal, it would be the logical library to use over Tomlj.

@kalinchan kalinchan merged commit 4b39582 into payara:main Oct 14, 2024
1 check passed
Pandrex247 pushed a commit to Pandrex247/Payara that referenced this pull request Oct 15, 2024
FISH-9198 Add Microprofile Config TOML Support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: DO NOT MERGE Don't merge PR until further notice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: TOML Config Source for MicroProfile Config /FISH-9198
4 participants