jboss-openshift / cct_module

CEKit modules shared by OpenShift container images
Apache License 2.0
46 stars 79 forks source link

Refactor Maven, add maven-openjdk8 module #372

Closed jmtd closed 4 years ago

jmtd commented 4 years ago

This is WIP so you can get an early look at what I'm thinking. I'm still performing extensive testing.

Ultimately this is all in aid of adding a module/version that will install maven-openjdk8 specifically (for use on RHEL 8.2+)

Based on top of #371

A brief summary of what i've done here

My starting point was "this is really confusing can't we have a single maven module name?" and I got as close as I could. jboss.container.maven.default could now potentially be merged into jboss.container.maven.api.

jmtd commented 4 years ago

I've un-WIPPED this. I think it's OK if a bit dramatic. In particular I'd like feedback on the proposed version scheme. TIA!

spolti commented 4 years ago

why remove the bats tests?

luck3y commented 4 years ago

@jmtd I like that there will only be one maven module, and while I don't like the version scheme looking at it initially, I don't really see a way to avoid this (the 8.2.3.6.8 one I had to re-read your comment after looking at the diff to remember what the last .8 .meant).

I agree with @spolti though, we use bats quite a lot for testing pull requests to jboss-eap-modules etc, so I'd recommend keeping them if you can (we can set up PR tests for this repo if you like.)

jmtd commented 4 years ago

If I’ve deleted the bats tests it’s a mistake and I’ll revert that bit. But I think you’re seeing one copy of them deleted in the diff from the merge of JBoss.container.maven.default.* and the other copy is still there

jmtd commented 4 years ago

If I’ve deleted the bats tests it’s a mistake and I’ll revert that bit. But I think you’re seeing one copy of them deleted in the diff from the merge of JBoss.container.maven.default.* and the other copy is still there

Yep just checked, jboss/container/maven/default/bash/default/tests/bats is the new home for them.

jmtd commented 4 years ago

@jmtd I like that there will only be one maven module, and while I don't like the version scheme looking at it initially, I don't really see a way to avoid this (the 8.2.3.6.8 one I had to re-read your comment after looking at the diff to remember what the last .8 .meant).

Thanks for your feedback! Yes, the version scheme leaves a bad taste in my mouth. If we could use dashes as well as dots it would look a lot better, but alas PEP440 does not support that.