search

repaint-io / maven-tiles

Injecting maven configurations by composition rather than inheritance
154 stars 32 forks source link

The change to "tiles-keep-id" (from attribute to child) breaks people upgrading from 2.12 to 2.14 #99

Closed rbygrave closed 5 years ago

rbygrave commented 5 years ago

The change introduced here by @bvella :

https://github.com/repaint-io/maven-tiles/commit/8c3f312a752074f0acf2494ff9432705d1b4762f#diff-0097a691dfec7c206bb0bcb8e6ca8dfdR87

In fixed rewriting execution ids and rewrite them in profiles as well changed from reading tiles-keep-id as an attribute of configuration to a child of configuration.

The impact of this is that any existing tiles relying on that functionality are now broken when upgrading from 2.12 to 2.14.

Was that intentional? Should not breaking changes have bumped the major version?

Is there a reason why we can not check for tiles-keep-id as both an attribute or a child of configuration?

Thanks, Rob.

rbygrave commented 5 years ago

Specifically it was reading attribute

if (configuration?.getAttribute("tiles-keep-id") == "true")

And that changed to reading child

if (execution.configuration?.getChild("tiles-keep-id")?.getValue() == "true")
bvella commented 5 years ago

Hi @rbygrave, you're right, I wanted to fix this in profiles, as it was not being applied, and for some reason, I interpreted it as being a child (which now in hindsight is a bit stupid on my part). I guess since there was no documentation or testing, once i convinced myself that is a child element I kept going with it. Pity it was not caught before the pull request was merged.

I will create a pull request that fixes this. At this point I think the safest way is to support both options, for the sake of anyone that starts to use it in the way it is now.

Apologies for the mistake. Hopefully the maintainers can release a new version soon

rvowles commented 5 years ago

I can if Mark is a bit slack.

On Tue, Jun 18, 2019 at 8:16 PM Brian Vella notifications@github.com wrote:

Hi @rbygrave https://github.com/rbygrave, you're right, I wanted to fix this in profiles, as it was not being applied, and for some reason, I interpreted it as being a child (which now in hindsight is a bit stupid on my part). I guess since there was no documentation or testing, once i convinced myself that is a child element I kept going with it. Pity it was not caught before the pull request was merged.

I will create a pull request that fixes this. At this point I think the safest way is to support both options, for the sake of anyone that starts to use it in the way it is now.

Apologies for the mistake. Hopefully the maintainers can release a new version soon

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/repaint-io/maven-tiles/issues/99?email_source=notifications&email_token=AAANTWHUJQWI3RAYLBIJMLTP3CKVJA5CNFSM4HY5DKJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX5SMZQ#issuecomment-502998630, or mute the thread https://github.com/notifications/unsubscribe-auth/AAANTWCRQA5JYATC6SI5MWTP3CKVJANCNFSM4HY5DKJQ .

--

Richard Vowles, Full stack - from Kubernetes, through Node & Java, Web and Mobile development in Flutter - software developer for hire!

ph: +64275467747, web: www.google.com/+RichardVowles

rbygrave commented 5 years ago

Awesome.

At this point I think the safest way is to support both options

Yes, that is what I was thinking too. Sounds good to me. Let me know if you get busy and want me to do the PR etc.

bvella commented 5 years ago

PR done #100

rvowles commented 5 years ago

Travis is hating it :-)

On Tue, Jun 18, 2019 at 8:38 PM Brian Vella notifications@github.com wrote:

PR done #100 https://github.com/repaint-io/maven-tiles/pull/100

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/repaint-io/maven-tiles/issues/99?email_source=notifications&email_token=AAANTWHQHOVQLP6TWRAFA4DP3CNG3A5CNFSM4HY5DKJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX5UKCA#issuecomment-503006472, or mute the thread https://github.com/notifications/unsubscribe-auth/AAANTWHNYKWV2QCZ7BUJNC3P3CNG3ANCNFSM4HY5DKJQ .

--

Richard Vowles, Full stack - from Kubernetes, through Node & Java, Web and Mobile development in Flutter - software developer for hire!

ph: +64275467747, web: www.google.com/+RichardVowles

bvella commented 5 years ago

commented on pr. test failure due to https://issues.apache.org/jira/browse/MNG-6636. It is not caused by these changes. I think travis builds with the latest maven and this issue affects 3.6.1. Apparently, @talios already raised this issue https://issues.apache.org/jira/browse/MNG-6647

rbygrave commented 5 years ago

Closing fixed in version 2.15 via #100

Edit: Not sure if we want to create milestone 2.15 and assign this ticket to it.

talios commented 5 years ago

I'm actually thinking of removing the attribute support actually. Mixing configuration elements and attributes is a sin, and doesn't really play well with some tooling.