Skip to content
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

Modulesync 5.1.0 #1125

Merged
merged 14 commits into from
Dec 7, 2021
Merged

Modulesync 5.1.0 #1125

merged 14 commits into from
Dec 7, 2021

Conversation

smortex
Copy link
Member

@smortex smortex commented Oct 27, 2021

No description provided.

@smortex smortex force-pushed the modulesync branch 4 times, most recently from 95b0d6b to 7207d36 Compare November 26, 2021 20:54
@smortex smortex changed the title Modulesync 4.2.0 Modulesync 5.1.0 Nov 27, 2021
@smortex smortex force-pushed the modulesync branch 20 times, most recently from 35cb192 to 778ae4a Compare December 1, 2021 03:51
@smortex
Copy link
Member Author

smortex commented Dec 1, 2021

I usually pin on the current version as lowest supported one. 1.0.0 will probaby not work.

It used to be like this at some point in the past (e.g. 040dcd9, 2dd4af0). In fact it is only required for the acceptance tests, the module (at least pretend to) do not care about how java is available… But I think it is less spaghetti to add it to metadata.json that to install it manualy at some point in the acceptance test suite initialization?

@ghoneycutt
Copy link
Member

There are a lot of open PR's for this module and after this sync occurs, rebasing will be very difficult for those contributors. Before merging this, suggest taking a pass through the PR's and merge as much as possible.

@smortex smortex marked this pull request as ready for review December 1, 2021 20:24
@smortex
Copy link
Member Author

smortex commented Dec 1, 2021

@ghoneycutt Thanks for helping sorting this out, I was focusing on this giant pile of work! I rebased on top of master and cleaned the commits the best I could, but because of a lot of intricate changes it is not as readable as I would have liked it to be 😢

I consider this ready for review now that CI is passing again in GitHub actions and without tests removal!

@alexjfisher
Copy link
Member

How confident are you with the rubocop changes? (It might be too much work now, but splitting rubocop fixes into at least safe autocorrect, unsafe autocorrect and manual fixes commits makes it much easier to review).
In the past, I may have gone a little overboard... voxpupuli/puppet-jenkins#831 https://github.com/voxpupuli/puppet-jenkins/pulls?q=is%3Apr+author%3Aalexjfisher+is%3Aclosed+rubocop :)

@smortex
Copy link
Member Author

smortex commented Dec 2, 2021

How confident are you with the rubocop changes?

Quite confident actually 😄

It is not visible due to commits cleanup and merging lots of FIXUP commits, but I incrementally adjusted the code until the test unit test suite pass, then fixed rubocop, then checked unit tests where still happy, then updated the voxpupuli-tests gem to fix more rubocop issues, then fixed the non-automatic fixable issues, and only when unit tests where green again attempted to fix the acceptance tests.

I tried to merge all related changes in single commits but the loads of changes induced by rubocop made it barely impossible to move the code at the correct location and some commits really deserved to be moved :-/. A trade-of was to merge some rubocop changes to have less conflicts to handle for moving these commits in history.

8852a0b is mostly automatic changes before voxpupuli-test update
efcc297 is mostly manual changes after the update

The repository was renamed to match the module name.  Use the new URL.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants