-
Notifications
You must be signed in to change notification settings - Fork 41.2k
Immutable configuration properties not shown by Actuator #18659
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
Immutable configuration properties not shown by Actuator #18659
Conversation
5770436
to
20e24ed
Compare
@snicoll Could you please spare time to look at it? I don't know why this kind of failure occurs. I tested all cases of jarcommandit locally, and they can pass. |
@polarbear567 I am sure you have the best intention but there is no need to at me for a review, the PR is open and someone from the team will have a look to it when time permits. |
Hi, @snicoll I think you misunderstood me. It's not that this PR needs your review, but that there is a problem with the CI build of spring-boot(like this: https://ci.spring.io/builds/93431). I want to ask you to help me to have a look this problem. After that, I tried again and passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I've added a few comments and we would need tests to cover the additional scenarios this is supposed to fix. We'd like to get a fix in 2.2.1
so let us know if you have time to rework this.
...g/springframework/boot/actuate/context/properties/ConfigurationPropertiesReportEndpoint.java
Outdated
Show resolved
Hide resolved
...g/springframework/boot/actuate/context/properties/ConfigurationPropertiesReportEndpoint.java
Outdated
Show resolved
Hide resolved
@snicoll Thank you for your opinion. I'd like to work this. It's complicated but interesting. |
Sorry to click the re-review button first. At present, it is judged according to the parameters in the constructor. There are many situations for the application of this annotation. I have considered all situations as much as possible, but I am not sure whether there will be any omissions. |
@polarbear567 every commit you push sends a notification to a large number of watchers. In the interest of keeping those to an acceptable level, please refrain from pushing isolated commits and, in particular, commits to trigger a new build. The build should work for you locally before submitting changes and we can figure out on our side what led to the build failure when reviewing. |
@snicoll Sure, I will notice this. And I'm not sure how to rebuild cli build, because my local build was successful |
@polarbear567 thanks for the efforts thus far but I think this is more involved than I thought initially and the lack of tests in this PR makes it hard to track what is covered and what isn't. I've started to roll-out a solution of my own and I've a number of tests that are covering the change. Given the complexity and potential change in other part of the codebase I am going to close this one. Thanks anyway. |
After testing, I found the problem was on the serializer modifier, so I made this change.