-
Notifications
You must be signed in to change notification settings - Fork 41
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
[MSITE-829] Upgrade Jetty to 9.4.x implicit java8 requirement now #21
Conversation
@@ -196,11 +196,11 @@ under the License. | |||
|
|||
<properties> | |||
<mavenVersion>3.0</mavenVersion> | |||
<javaVersion>7</javaVersion> | |||
<javaVersion>8</javaVersion> |
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.
This change should probably be made, if at all, in its own PR, not as a driveby of a minor version dependency update
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.
we need 8 for the change so let's do it all together
except having a sort of bureaucratic own PR I cannot see the technical need :)
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.
@olamy I am the bureaucratic person. This needs to be changelog visible.
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.
it's ALREADY there https://issues.apache.org/jira/browse/MSITE-828 and the git comment will clearly says that
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.
Good, let's re-merge MSITE-828 first.
@@ -623,6 +624,7 @@ under the License. | |||
<maven.compiler.source>${maven.compiler.source}</maven.compiler.source> | |||
<maven.compiler.target>${maven.compiler.target}</maven.compiler.target> | |||
</properties> | |||
<javaHome>${java.home}</javaHome> |
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.
Why is that necessary?
@@ -76,7 +76,7 @@ under the License. | |||
<plugin> | |||
<groupId>org.apache.maven.plugins</groupId> | |||
<artifactId>maven-javadoc-plugin</artifactId> | |||
<version>2.7</version> | |||
<version>@javadocPluginVersion@</version> |
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.
How are these related?
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.
I use java11 so the IT tests were failing for locally because of this and need more recent java version
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.
Makes sense, but should be a separate PR because it is logically not related.
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.
sorry but I will not do it. Maybe I should create a Jira ticket as well.
Seriously stop such nit picking... I prefer spend my time on more useful stuff for the project than waste my time remove this commit creating a branch and a separate PR only for this...
I don't see why it's useful for the project.
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.
+1 on the JIRA ticket. If you are not willing to clean up. Leave as-is and have someone else clean it up. No issue.
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.
LOL thanks you made my day
249b4a8
to
64d4100
Compare
Hi, what's blocking this? 9.2.29 is not safe anymore |
Not safe for what? |
Sorry, my initial comment was vague. There are a couple of security vulnerabilities against Jetty, such as : CVE-2021-28165 Read more : https://www.cybersecurity-help.cz/vdb/SB2021040179 |
and none of them affect this plugin if you'd knew what we do with Jetty. |
That makes sense. Problem is that security scans do not have the background about how this is used within the plugin and they simply block the build when the plugin tries to pull Jetty |
Therefore, I consider them partially useless. Just like this. Maybe someone can rework the PR. |
Somewhat. They block Jetty for everyone(including the projects where the vulnerabilities applies) which affects this plugin indirectly. If it helps, what we use is similar to this : If this PR is considered stale then I can resume and maybe target the latest version instead? |
Many vendors provide this superficial crap -- as you can see it proves nothing here.
Split between Java 8 upgrade and Jetty upgrade in at least two PRs. @hboutemy @rfscholte Yet another reason why we need to split this plugin in two. |
@michael-o @yeikel
|
Definitely. We had to overwrite the version manually in our build to be able to use the plugin but doing so without the corresponding tests could introduce unexpected regressions for us |
625ee31
to
88aa108
Compare
I know and that is sadly stupid. |
|
@olamy builds fails only on Maven 3.8.2, same on the master branch |
@slachiewicz I saw and I guess because of https://issues.apache.org/jira/browse/MNG-7215 |
All GitHub runners will have Maven |
Sorry to bump, but what happened to this? |
I rebased from master. |
Did you get a chance to work on this? |
- build from 8 only and we do not support anymore 3.0.x - improve gh action Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
No description provided.