You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by Lemongrass3110 <gi...@git.apache.org> on 2018/05/03 14:08:44 UTC
[GitHub] logging-log4j2 pull request #173: [LOG4J2-2332] Unresolved variables in any ...
GitHub user Lemongrass3110 opened a pull request:
https://github.com/apache/logging-log4j2/pull/173
[LOG4J2-2332] Unresolved variables in any file appender
Proposed fix for LOG4J2-2332
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Lemongrass3110/logging-log4j2 hotfix/LOG4J2-2332
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/logging-log4j2/pull/173.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #173
----
commit 538322198b1a94d1b2a94a799d64098db6f7d3d3
Author: Lemongrass3110 <le...@...>
Date: 2018-05-03T14:04:04Z
Proposed fix for LOG4J2-2332
----
---
[GitHub] logging-log4j2 issue #173: [LOG4J2-2332] Unresolved variables in any file ap...
Posted by jvz <gi...@git.apache.org>.
Github user jvz commented on the issue:
https://github.com/apache/logging-log4j2/pull/173
Travis is broken due to some integration tests that don't seem to work well in CI builds currently, don't worry too much about it affecting your PR. I manually verify a PR builds before merging to master as it is.
As for making it default behavior, I personally would prefer to adopt this new behavior as well, though I'm not sure how it would affect existing configurations. If it's not adopted as default behavior, perhaps a system property addition to globally override that setting?
---
[GitHub] logging-log4j2 issue #173: [LOG4J2-2332] Unresolved variables in any file ap...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on the issue:
https://github.com/apache/logging-log4j2/pull/173
```@RequiresVariables``` is not quite right either IMO, ```@RequiresResolvedVariables``` might be better.
---
[GitHub] logging-log4j2 issue #173: [LOG4J2-2332] Unresolved variables in any file ap...
Posted by jvz <gi...@git.apache.org>.
Github user jvz commented on the issue:
https://github.com/apache/logging-log4j2/pull/173
Hey, first of, thanks for the contribution! Personally, I'm excited to see someone noticed the validation API in the first place. :D
Anyways, I certainly like this idea. In fact, this actually solves a long standing annoyance I've had with the same issue in the past.
---
[GitHub] logging-log4j2 issue #173: [LOG4J2-2332] Unresolved variables in any file ap...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on the issue:
https://github.com/apache/logging-log4j2/pull/173
We should consider if the presence of unresolved variables should:
- Log a DEBUG, WARN, ERROR, or FATAL messages
- Fail the the whole config
---
[GitHub] logging-log4j2 issue #173: [LOG4J2-2332] Unresolved variables in any file ap...
Posted by rgoers <gi...@git.apache.org>.
Github user rgoers commented on the issue:
https://github.com/apache/logging-log4j2/pull/173
Why not just an attribute of the existing plugin?
---
[GitHub] logging-log4j2 issue #173: [LOG4J2-2332] Unresolved variables in any file ap...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on the issue:
https://github.com/apache/logging-log4j2/pull/173
Thank you for your contribution.
I am not a fan of the name "@NoUnresolvedVariables" because of the two negatives "No" and the "Un" in "Unresolved". Can we come up with a simpler name? Maybe "@ValidateVariables".
I also wonder if the annotation should have an optional log level so that not ALL validation failures end up as an ERROR event.
---
[GitHub] logging-log4j2 issue #173: [LOG4J2-2332] Unresolved variables in any file ap...
Posted by Lemongrass3110 <gi...@git.apache.org>.
Github user Lemongrass3110 commented on the issue:
https://github.com/apache/logging-log4j2/pull/173
@jvz I debugged my way through and stumbled over it while doing so. I actually wondered why some `@Required` tags are not there for some of the file loggers "file" attribute, while they are there in other cases.
Personally I do not really care for whatever name you guys decide on, it is just the functionality I need. My point of view is just that it sucks to run a java program with priviledges that allow you to create directories and end up creating directories all over the place where you do not actually want them with really weird names.
Regarding backwards compatibility I do not really know if this is a reason to take into account since this is as you said yourself actually a configuration issue that goes around unnoticed for now and should actually be reported/validated before calling mkdirs on it. It might just bring up configuration files that were actually broken all time long and should have been fixed anyway. But those are just my few cents on this topic.
If you decide to make this non-default behaviour:
Is there a way I can make it default behavior without using a configuration file? Because I supply my customers with a framework that uses log4j2 as it's logging utility and they can supply configuration files I do not have any control over and I want to prevent them messing up their directories unintentionally.
I would have been happy enough if I could have intercepted the mkdirs call somehow and cancel it, since I am validating the appenders target directory after the context initialization, but it is too late at this point already.
---
[GitHub] logging-log4j2 issue #173: [LOG4J2-2332] Unresolved variables in any file ap...
Posted by rgoers <gi...@git.apache.org>.
Github user rgoers commented on the issue:
https://github.com/apache/logging-log4j2/pull/173
@garydgregory Agreed. That was what I was questioning in my first comment.
---
[GitHub] logging-log4j2 issue #173: [LOG4J2-2332] Unresolved variables in any file ap...
Posted by Lemongrass3110 <gi...@git.apache.org>.
Github user Lemongrass3110 commented on the issue:
https://github.com/apache/logging-log4j2/pull/173
I think it should at least be error level, since during context initialization by default only StatusLogger is used and only level error or higher is reported to stderr(correct me if I am wrong please).
As for the rest I will leave it uncommented since you guys probably know better how you handled similar problems in the past.
---
[GitHub] logging-log4j2 issue #173: [LOG4J2-2332] Unresolved variables in any file ap...
Posted by jvz <gi...@git.apache.org>.
Github user jvz commented on the issue:
https://github.com/apache/logging-log4j2/pull/173
Maybe `@RequiresVariables` or something like that?
And all the validation annotations so far have all been used to err a plugin factory which seems like a config error to me. As a user, I'd like to see errors related to where I messed up most of all.
---
[GitHub] logging-log4j2 issue #173: [LOG4J2-2332] Unresolved variables in any file ap...
Posted by Lemongrass3110 <gi...@git.apache.org>.
Github user Lemongrass3110 commented on the issue:
https://github.com/apache/logging-log4j2/pull/173
Btw. could you guys help me with the travis issues? There are a lot of exceptions I am seeing in there and I do not know which are intentional and which I actually caused and which are causing travis to fail the build. If you give me a few hints I will gladly look into them.
I have also enabled edits by maintainers, so feel free to push fixes/changes to my fork/this pull request.
---
[GitHub] logging-log4j2 issue #173: [LOG4J2-2332] Unresolved variables in any file ap...
Posted by jvz <gi...@git.apache.org>.
Github user jvz commented on the issue:
https://github.com/apache/logging-log4j2/pull/173
The way validation constraints work is that it fails the plugin only (will return null along with logging the error). It was originally developed to automate all the null/isEmpty() checks everywhere via the `@Required` annotation and we've added more since then.
Anyways, it could be implemented an attribute of `@Required` in that case. For backwards compatibility reasons, I wouldn't make that the default state to avoid breaking existing configs.
---
[GitHub] logging-log4j2 issue #173: [LOG4J2-2332] Unresolved variables in any file ap...
Posted by rgoers <gi...@git.apache.org>.
Github user rgoers commented on the issue:
https://github.com/apache/logging-log4j2/pull/173
This shouldn't need to be a system property. it can be an attribute of the configuration element.
---