8394 maven mods #8395
8394 maven mods #8395
Conversation
poikilotherm
requested review from
pdurbin,
landreev,
akio-sone,
scolapasta and
qqmyers
|
I have no idea why this won't build on Jenkins - cannot look in the logs, either. Sorry. Unit tests via GH Action are doing OK |
poikilotherm
force-pushed
the
8394-maven-mods
branch
from
96fde95 to
90a1d96
Compare
poikilotherm
added
Component: Code Infrastructure
|
Hey @landreev I just took to opportunity to refactor the zipper submodule with the parent to extend this example a bit further, outlining further benefits. In the same go slightly refactored the packaging to use Spring Boot Fat JAR generation instead of Maven Assembly. (And maybe talk about releasing this component? As it's in an experimental status, it might be worth changing the version to sth <1.0?) (BTW: the idea is great - now it would be very nice to use the zipper without |
|
Apologies for the delay. I wanted to build the zipper (it builds for me w/out problems, yes) and test it a bit (still need to do some more of that). The refactoring generally seems ok. I'm not sure about the name change; I do like your "zipdownloader" better - but seeing how we mentioned it under the old name, and it would need to be changed - I'm not sure it's worth it. As for releasing it... in all honesty, I'm very apprehensive about advertising it to other installations as an integral part of the application. In its current form it's a big hack; (by design, but a hack nevertheless). If we wanted to release it and make it an official, recommended application component - and not just something a very adventurous installation can choose to use at their own risk, I would revisit certain solutions there first. (We can certainly talk about it some more outside of this issue). |
|
As for the rest of the PR, I'm still seeing things like |
…QSS#8394 This includes moving/inheriting some project properties from the parent.
|
@poikilotherm and I iterated a bit on poikilotherm#524 and I hope it's ready to merge. Meanwhile, why is this pull request still in draft? Are we waiting for something? At standup yesterday I believe we said we're waiting to hear a bit more about the vision for this change. I'm not sure what form this would take. A discussion at tech hours? More docs? A comment here? Finally, I'll just add that I was able to build and deploy this branch in docker-aio. So nothing seems to be broken, from a quick check. I appreciate that the parent pom is on the side and that we don't have to immediately change how we do things, where we invoke I guess I'll leave myself on this pull request for now but I don't have any more to say. I think it's ok to merge (but again, I'd like poikilotherm#524 to be merged first). |
doc tweaks for IQSS#8394
|
@pdurbin this is still in draft as there might be things left you folks don't like, miss, want changed etc. And maybe let's talk about this stuff first before creating facts and revokes if consensus says we don't wanna go down this road. |
|
@poikilotherm sounds good. From standup, it sounds like we're looking forward to you talking a bit about this pull request at a future tech hours before we merge. I'll assign this to you for now. Thanks for merging my doc change. I don't think I have anything else to add so I'll unassign myself. @landreev mentioned he had a least a doc change to make. |
Mentioned the name change; removed the link to the pre-built binary distribution in the 5.0 release.
I'm assuming that was unintentional, splitting the paragraph in two. (Especially with the sentence beginning with "Thus...").
|
@poikilotherm are you ready to move this PR out of Draft status? Yesterday at standup @landreev and I said we think it can go to QA. |
…en plugin IQSS#8394" This reverts commit 424400b. We believe this is causing our Jenkins job to fail. It's showing this: [JaCoCo plugin] Overall coverage: class: 0.0, method: 0.0, line: 0.0, branch: 0.0, instruction: 0.0, complexity: 0.0 When we expect something more like this: [JaCoCo plugin] Overall coverage: class: 73.52321, method: 46.495075, line: 41.614933, branch: 33.08663, instruction: 41.725876, complexity: 31.795692
|
@poikilotherm heads up that in 09167d5 I reverted a Jacoco-related commit that may be the reason the Jenkins job is failing. I left details in the comment. |
|
Oh hey great thanks for merging! Pretty cool you folks let me introduce this change 😊 I promise to be helpful when question arise in the future 🙈✌️ |
What this PR does / why we need it:
This PR will introduce a new Maven parent POM and refactor the current main POM to use it. Basically nothing really changes from this refactoring despite the fact that we may rely on this new module to coordinate dependency versions, dependency convergence, importing common BOMs etc.
This PR also adds a filter to the Github Action to run unit tests when the main Dataverse POM and the POMs in the modules are changed. This might be even changed to make the unit tests run on the parent POM, so it picks up all modules. (@donsizemore WDYT?)
Please note: this PR maybe should incorporate changes to include the POM for ZIP downloader as a submodule. As it might make sense to create another module in
module/dataverse-zipdownloaderor similar, this might also live within another pull request. Small steps, wholesome stuff - whatever you prefer.Which issue(s) this PR closes:
Relates to #8394
(Not using "Closes" here because of the remaining things like refactoring ZIP downloader)
Special notes for your reviewer:
None so far. Please add any suggestions, opinions etc. Should we add some doccs in the dev guide -> dependency management? What about adding some hints on architecture, as this might be a start to move Dataverse monolith towards a modulith?
Suggestions on how to test this:
Use Maven in project root, watch it do as it always has. Go to
modules/dataverse-parentand use Maven there, see the difference (submodules detected and used).Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope.
Is there a release notes update needed for this change?:
Not yet. Please add suggestions on this matter...
Additional documentation:
None yet.
closes #8394