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

Unpin Logback version #279

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Unpin Logback version #279

wants to merge 3 commits into from

Conversation

lbwexler
Copy link
Member

@lbwexler lbwexler commented Mar 12, 2023

We can take this now, but note that it will require our applications to rename their logback.groovy files. There might need to be an additional rename back to logback.groovy in the future, which would be annoying.

Alternatively we could wait a tick, untils grails upgrades to the lastest version of logback, and it looks like then the file rename would not be needed. Guess it depends on how anxious we are to unping logback.groovy.

See matching toolbox branch here

xh/toolbox#644

@lbwexler lbwexler requested a review from amcclain March 12, 2023 19:55
@lbwexler lbwexler linked an issue Mar 12, 2023 that may be closed by this pull request
Copy link
Member

@amcclain amcclain left a comment

Choose a reason for hiding this comment

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

Apols for letting this linger for so long w/o comment. Ryan just ran into this while updating a Hoist app - needed to add the magic logback.version=1.2.7 to gradle.properties.

Not a big hassle, but one more bit of boilerplate spread across apps, and the kind of thing that will likely get caught in various scans, etc. going forward. So am motivated to merge if we have a look at the latest from dogbert and determine it's all working as expected.

* v16 restores the use of the standard grails version of logback (v1.2.11). Hoist had been
pinned on an earlier version of logback due to dropped support of groovy based configuration.
This support is now restored via plugin. This change will require renaming the
`conf/logback.groovy` file in your application to `conf/logback-config.groovy`. It also may
Copy link
Member

Choose a reason for hiding this comment

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

Note that this is no longer the case - can stick with logback.groovy as per updated dogbert docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, not that simple (see below)

@@ -76,6 +76,7 @@ dependencies {
api 'org.jasypt:jasypt:1.9.3'
api "commons-io:commons-io:2.8.0"
api "org.owasp.encoder:encoder:1.2.3"
implementation 'io.github.virtualdogbert:logback-groovy-config:1.0'
Copy link
Member

Choose a reason for hiding this comment

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

There are now different versions of this library depending on the particular minor version of logback we're looking to take.

@amcclain
Copy link
Member

(Happy to try to make the updates to the library version and tweak this PR - figured we could decide if we have any interest in doing that before the big multi-instance merge, or not)

@lbwexler
Copy link
Member Author

I looked at this a month or two back, but was frustrated to see that grails was still on a newer, but not latest version of logback. That version would require apps to rename the logback.groovy with this workaround. Unfortunately, we are unable to take the very latest version of logback as it is incompatible with grails.

Chatted with the author of the plugin (formerly known as virtualdogbert, now groovyduke) and he suggested we wait until grails upgrades logback to the latest.

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.

Unpin logback version
2 participants