Skip to content

Bug: Extra JARs and Artifacts were not subjected to filtering#785

Merged
cstamas merged 7 commits intomasterfrom
bugfix-extra-filter
Feb 26, 2026
Merged

Bug: Extra JARs and Artifacts were not subjected to filtering#785
cstamas merged 7 commits intomasterfrom
bugfix-extra-filter

Conversation

@cstamas
Copy link
Member

@cstamas cstamas commented Feb 24, 2026

Bug: the extraJars and extraArtifacts were added to shaded JAR as is, without subjecting them to filtering.

@cstamas
Copy link
Member Author

cstamas commented Feb 24, 2026

They used Java 1.5 and around as configured output, and
it simply does not work with modern Java. Lifted all
to Java 8.
IT should use Maven def plugin
Shade plugin starts to create DRP even if the artifact has been renamed because of the configuration
</description>

<properties>
Copy link
Contributor

Choose a reason for hiding this comment

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

switching to Java 8 feels like a separate issue. Is there some reason it's in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

As ITs are busted without this change, at least I could not make ITs work locally (on Java 24, 21, etc).

Copy link
Member

@olamy olamy left a comment

Choose a reason for hiding this comment

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

LGTM

@olamy olamy added the bug Something isn't working label Feb 24, 2026
@olamy olamy mentioned this pull request Feb 24, 2026
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

If all ITs are busted then that should be fixed first in a separate PR.

@olamy
Copy link
Member

olamy commented Feb 25, 2026

If all ITs are busted then that should be fixed first in a separate PR.

not sure to understand the reasoning? what would be the added value of adding extra work/waste of time with a separate PR??

It has been discovered the reason of the failure with Java 25 so it's fixed now.

@Bukama
Copy link
Contributor

Bukama commented Feb 25, 2026

TBH I would like to see my "update Parent 47" draft (#781), I opend two weeks ago, to get done to work and merged, because it a) does not hide those big changes behind a "bugfix" b) sets up ITs to use parent version for lesser maintanence effort in the future (idea/request by @slawekjaranowski ). I just could not get it "green" 100% alone.

Copy link
Contributor

@Bukama Bukama left a comment

Choose a reason for hiding this comment

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

Replace plugin versions in ITs with parent ones (see #781)

pom.xml Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove version. Was needed to fix Java 25 build in between, but compatible version is now part of parent

@olamy
Copy link
Member

olamy commented Feb 25, 2026

TBH I would like to see my "update Parent 47" draft (#781), I opend two weeks ago, to get done to work and merged, because it a) does not hide those big changes behind a "bugfix" b) sets up ITs to use parent version for lesser maintanence effort in the future (idea/request by @slawekjaranowski ). I just could not get it "green" 100% alone.

no worries. if you prefer this way. I was just thinking of expedite this and have it done and dusted.
Having a separate PR or not doesn't have any effect on users so up2u (it's just internal test which users doesn't know anything about)

@Bukama
Copy link
Contributor

Bukama commented Feb 25, 2026

TBH I would like to see my "update Parent 47" draft (#781), I opend two weeks ago, to get done to work and merged, because it a) does not hide those big changes behind a "bugfix" b) sets up ITs to use parent version for lesser maintanence effort in the future (idea/request by @slawekjaranowski ). I just could not get it "green" 100% alone.

no worries. if you prefer this way. I was just thinking of expedite this and have it done and dusted. Having a separate PR or not doesn't have any effect on users so up2u (it's just internal test which users doesn't know anything about)

Sure. Splitting it, would it make change history more clearer for us - the maintainers. For users of the plugin the whole parent thing doesn't matter at all. They just consume a compiled and rleased artifact.

@elharo
Copy link
Contributor

elharo commented Feb 25, 2026

Standard best practice. PRs should be small and focused on one thing.

https://google.github.io/eng-practices/review/developer/small-cls.html
https://testing.googleblog.com/2024/07/in-praise-of-small-pull-requests.html

In the specific case of Maven, it helps a lot with code archaeology in the future. It has not escaped my notice that on another PR in surefire right now we are unable to understand why changes were made in a PR that did several things and only documented one of them, and consequently progress is blocked.

@cstamas
Copy link
Member Author

cstamas commented Feb 26, 2026

I am about to merge this PR as:

  • the actual bugfix was approved by @olamy
  • the change requests by @Bukama and @elharo were about things not anymore present in this PR

I consider this PR "good to go"/

@cstamas cstamas merged commit eca6398 into master Feb 26, 2026
30 checks passed
@cstamas cstamas deleted the bugfix-extra-filter branch February 26, 2026 14:12
@github-actions github-actions bot added this to the 3.6.2 milestone Feb 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants