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

Enhancement: Hierarchical System Properties #130

Conversation

Traderjoe95
Copy link
Contributor

Fixes #129

@vietj
Copy link
Contributor

vietj commented Mar 31, 2021

can you document this in the index.adoc file ?

also I think we should change the hierarchical name to rather something that mentions that now qualified names are accepted.

@Traderjoe95
Copy link
Contributor Author

can you document this in the index.adoc file ?

I added the documentation and also added some assertions to the test cases. The test now asserts that the properties are indeed parsed to a flat object when hierarchical = false, and that the flat properties are still parsed correctly when hierarchical = true

also I think we should change the hierarchical name to rather something that mentions that now qualified names are accepted.

In my opinion, hierarchical explains the purpose of the attrbute pretty well. I'm not sure that something like qualified will be much clearer. However, maybe I just didn't come up with a good name yet. If we change this, it should also be changed on the PropertiesConfigProcessor to preserve consistent naming.

@Traderjoe95
Copy link
Contributor Author

I see the check is failing, but that doesn't seem to be related with my changes. There's some test failing in the vertx-config-vault module.

@vietj
Copy link
Contributor

vietj commented May 5, 2021

ok @Traderjoe95

@vietj
Copy link
Contributor

vietj commented May 5, 2021

I will check locally the tests

@vietj
Copy link
Contributor

vietj commented May 5, 2021

I fixed the spring config cloud server issues in the CI

@vietj
Copy link
Contributor

vietj commented May 5, 2021

the current vault failures are due to https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8266261

@vietj
Copy link
Contributor

vietj commented May 5, 2021

can you rebase and force push on latest master ? now tests should pass

@vietj
Copy link
Contributor

vietj commented May 6, 2021

ping @Traderjoe95

tsegismont and others added 3 commits May 6, 2021 15:27
Otherwise the Vert.x stack convergence tests fail (conflict with Cassandra Client)
@Traderjoe95 Traderjoe95 force-pushed the enhancement/hierarchical-system-properties branch from 0f93be1 to 98175d9 Compare May 6, 2021 13:30
@Traderjoe95
Copy link
Contributor Author

Hey, sorry it looks like I accidentally destroyed the history when rebasing. I will try to fix it

@Traderjoe95
Copy link
Contributor Author

Okay, the commits are back. Do you want me to clean up the history or is the merge commit fine?

@vietj
Copy link
Contributor

vietj commented May 6, 2021

@Traderjoe95 if you can squash commits and sign the ECA, it would be great

@Traderjoe95
Copy link
Contributor Author

Of course, will do.

Regarding the ECA, I already signed it. The failure is related to one of the commits I pulled in from master

Also adds tests and documentation

Signed-off-by: Johannes Leupold <johannes.leupold@kobil.com>
@Traderjoe95 Traderjoe95 force-pushed the enhancement/hierarchical-system-properties branch from be522ad to d4b78a1 Compare May 6, 2021 14:03
@Traderjoe95
Copy link
Contributor Author

image

Now the only issue remaining is this. I signed the ECA, but one of the rebased commits is missing.

Plus, I had some failing tests locally, but those didn't have anything in common with my changes. I want to see if it succeeds in the pipeline

@vietj
Copy link
Contributor

vietj commented May 10, 2021

can you provide a new PR created from master on which you cherry-pick your commits only ?

@Traderjoe95
Copy link
Contributor Author

Sure, I will create a new one during the day

@Traderjoe95
Copy link
Contributor Author

Closed in favor of #135

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Allow hierarchical parsing for system properties
3 participants