Update to license-maven-plugin:4.1 #550

brenuart · 2021-07-06T09:12:13Z

Update POM to use the latest version of the Mycila license plugin (and correct GAV).
Move plugin configuration inside the section so the plugin can be manually invoked from the command line. Particularly handy to fix header issues automatically…
Change the build process to “check” the files only and fail the build if not valid.

Related issue: #549

brenuart · 2021-07-06T09:16:44Z

I noticed that the license header is using the JAVADOC comment style:

/**   <---------
 * Licensed under the Apache License, Version 2.0 (the "License");
 * ...
 */

This means that a class may have TWO class-level javadoc sections: one with the license and a second with the actual class documentation. IMHO the license header should be enclosed in a standard comment block like this:

/* <--------
 * Licensed under the Apache License, Version 2.0 (the "License");
 * ...
 */

If you agree with that, I can easily update the header in all source files to conform with this style as part of this PR...

- Update POM to use the latest version of the Mycila license plugin (and correct GAV). - Move plugin configuration inside the <pluginManagement> section so the plugin can be manually invoked from the command line. Particularly handy to fix header issues automatically… - Change the build process to “check” the files only and fail the build if not valid. - Remove the custom lifecycle-mapping: latest versions of the plugin have support for M2E Related issue: logfellow#549

philsttr · 2021-07-06T16:50:20Z

Nice!

If you agree with that, I can easily update the header in all source files to conform with this style as part of this PR...

Yes, please do. I never noticed that.

philsttr · 2021-07-06T17:08:23Z

Oh, while you're modifying the license stuff... can you add a copyright to the header and each file to which it applied? like this:

 * Copyright 2013-2021 the original author or authors.

See also #536 and spring-projects/spring-boot#27021 (comment), and an example

According to that comment, leave the template value in LICENSE as-is, but just apply the copyright to the header, and each file.

brenuart · 2021-07-06T18:04:24Z

I can of course add the copyright in the header... but I'm a bit worried about the date range...
What should I use as end year?

(1) Is it the "current year"? In this case, should we update all files next year? This may not be an issue: the plugin can help us with this task!

(2) Is it the year of the last modification of the file? Then we need to configure the license plugin to get that information from Git... This is supported by the plugin but comes with many drawbacks. First performances are horrible. Second, you start having strange behaviour depending on whether the file is modified, staged, new or renamed. The rename is even more confusing as it depends on Git detecting the rename or not - and as you know this depends on ho similar the file is with another (and sometimes Git says the file is renamed although it is a brand new one)... We took that road at work for several years and finally decided to drop any "git" dates in the header!

philsttr · 2021-07-06T18:49:02Z

There is no perfect solution, but this problem isn't unique to logstash-logback-encoder.

So, we could follow the same practice as others do, like spring-boot.

As part of this one-time task, stamp everything with 2021.

Just keep an eye out for any pull requests, and update the headers manually then.

If that becomes a problem, we can manually bump the year next year. I'm not worried about a once-a-year maintenance task.

Change comment style from JAVADOC to SLASHSTAR Add copyright as first line

brenuart · 2021-07-06T19:30:50Z

Oh... this odd - StackHasherTest#expected_hash_should_be_generated() is failing because of the changes in the license header??? It passes when I revert the header back to what it was before...

Here is part of the stacktrace:

org.opentest4j.AssertionFailedError: expected: <e30d4cae> but was: <48abcbb0>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1124)
	at net.logstash.logback.stacktrace.StackHasherTest.expected_hash_should_be_generated(StackHasherTest.java:156)

Should I just change the expected value to 48abcbb0 to make it happy ?

brenuart · 2021-07-06T20:57:00Z

Maybe we should remove the header check from the checkstyle rules as well?
Was:

    <module name="RegexpHeader">
        <property name="header" value="^/\*\*\n^ \* Licensed under the Apache License, Version 2\.0 \(the &quot;License&quot;\);"/>
    </module>

philsttr · 2021-07-07T00:20:43Z

StackHasherTest#expected_hash_should_be_generated() is failing because of the changes in the license header???

I'm guessing the hash changed because of line number changes (?)

Should I just change the expected value to 48abcbb0 to make it happy ?

Yes

Maybe we should remove the header check from the checkstyle rules as well?

I kind of like keeping the checkstyle rule, since checkstyle plugins in IDEs will highlight the problem (vs having to wait until a maven build)

…nal lines in the license header

brenuart · 2021-07-07T10:25:23Z

I kind of like keeping the checkstyle rule, since checkstyle plugins in IDEs will highlight the problem (vs having to wait until a maven build)

I see the point.
Unfortunately, the rule is now failing since we changed the license header. Comment style is different (minor issue) - but first line is different as well as it now includes the "current year".
I changed the rule to match on the first few lines of the header - ignoring the "year" part. This also means that the lines are now duplicated in both the template AND the rule itself.

brenuart · 2021-07-07T10:37:39Z

Build is still failing but because of the flaky LogstashTcpSocketAppenderTest.testConnectOnPrimary:243 - this test fails from time to time - I should definitely have a look at it and make it more reliable...
Could you please restart the build ?

philsttr · 2021-07-07T16:12:20Z

Done. And filed #560 to address flaky test

philsttr · 2021-07-07T16:13:54Z

pom.xml

        <maven-bundle-plugin.version>5.1.2</maven-bundle-plugin.version>
        <maven-checkstyle-plugin.version>3.1.2</maven-checkstyle-plugin.version>
        <maven-compiler-plugin.version>3.8.1</maven-compiler-plugin.version>
        <maven-enforcer-plugin.version>1.4.1</maven-enforcer-plugin.version>
        <maven-gpg-plugin.version>3.0.1</maven-gpg-plugin.version>
        <maven-jar-plugin.version>3.2.0</maven-jar-plugin.version>
        <maven-javadoc-plugin.version>3.3.0</maven-javadoc-plugin.version>
-        <maven-license-plugin.version>1.9.0</maven-license-plugin.version>
+        <license-maven-plugin.version>4.1</license-maven-plugin.version>


nit put this in alpha order

CONTRIBUTING.md

brenuart force-pushed the gh549 branch from 4d14e14 to 6a83f05 Compare July 6, 2021 09:20

brenuart force-pushed the gh549 branch from 6a83f05 to f95da99 Compare July 6, 2021 09:21

Update license header

d1494e8

Change comment style from JAVADOC to SLASHSTAR Add copyright as first line

Update test case with a different expected hash caused by the additio…

7228677

…nal lines in the license header

Update checkstyle rule to reflect the new license header

65f3c5c

philsttr reviewed Jul 7, 2021

View reviewed changes

Sort properties in alpha order

d53b401

philsttr linked an issue Jul 7, 2021 that may be closed by this pull request

Bump license-maven-plugin to 4.1 (from 1.9.0) #549

Closed

philsttr reviewed Jul 7, 2021

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

Fix http link in CONTRIBUTING.md

a64bc73

philsttr linked an issue Jul 7, 2021 that may be closed by this pull request

Copyright Notice in License is missing #536

Closed

philsttr merged commit b1ec1db into logfellow:main Jul 7, 2021

philsttr added this to the 7.0 milestone Jul 7, 2021

brenuart deleted the gh549 branch July 7, 2021 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to license-maven-plugin:4.1 #550

Update to license-maven-plugin:4.1 #550

brenuart commented Jul 6, 2021

brenuart commented Jul 6, 2021

philsttr commented Jul 6, 2021

philsttr commented Jul 6, 2021

brenuart commented Jul 6, 2021 •

edited

Loading

philsttr commented Jul 6, 2021

brenuart commented Jul 6, 2021 •

edited

Loading

brenuart commented Jul 6, 2021

philsttr commented Jul 7, 2021

brenuart commented Jul 7, 2021 •

edited

Loading

brenuart commented Jul 7, 2021

philsttr commented Jul 7, 2021

philsttr Jul 7, 2021

Update to license-maven-plugin:4.1 #550

Update to license-maven-plugin:4.1 #550

Conversation

brenuart commented Jul 6, 2021

brenuart commented Jul 6, 2021

philsttr commented Jul 6, 2021

philsttr commented Jul 6, 2021

brenuart commented Jul 6, 2021 • edited Loading

philsttr commented Jul 6, 2021

brenuart commented Jul 6, 2021 • edited Loading

brenuart commented Jul 6, 2021

philsttr commented Jul 7, 2021

brenuart commented Jul 7, 2021 • edited Loading

brenuart commented Jul 7, 2021

philsttr commented Jul 7, 2021

philsttr Jul 7, 2021

Choose a reason for hiding this comment

brenuart commented Jul 6, 2021 •

edited

Loading

brenuart commented Jul 6, 2021 •

edited

Loading

brenuart commented Jul 7, 2021 •

edited

Loading