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

BREAKING: Enable packagecloud repos by default #926

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kajinamit
Copy link
Contributor

Pull Request (PR) description

The packagecloud repo was enabled by default in old releases but it was disabled by #493 .
Now we need this repo to support CentOS 8 , and we discussed this behavior and agreed this should be enabled by default.
Further details can be found in #900 .

This Pull Request (PR) fixes the following issues

N/A

@wyardley
Copy link
Contributor

Hi @kajinamit sorry for the delay in response.
These days, it looks like we want the reference.md to be regenerated (which is causing the initial CI checks to fail, which means we can't see how the other acceptance tests look in CI). Can you update it (I think it's bundle exec rake strings:generate:reference) and push? I'll also take a quick look through the changes and see if there's anything obvious that really strikes me.

Eventually we'll want this to be one squashed commit, ideally.

@kajinamit
Copy link
Contributor Author

Hi @kajinamit sorry for the delay in response. These days, it looks like we want the reference.md to be regenerated (which is causing the initial CI checks to fail, which means we can't see how the other acceptance tests look in CI). Can you update it (I think it's bundle exec rake strings:generate:reference) and push? I'll also take a quick look through the changes and see if there's anything obvious that really strikes me.

Eventually we'll want this to be one squashed commit, ideally.

Thanks for the pointer ! I regenerated reference.md and squashed that change into the base one which changed the default/parameter description.

@wyardley
Copy link
Contributor

wyardley commented Apr 7, 2023

Hi @kajinamit
sorry for not commenting again sooner. Can you take a look at the failing spec tests?

@wyardley wyardley mentioned this pull request May 10, 2023
@wyardley
Copy link
Contributor

@kajinamit also, if you haven't already found it:
https://github.com/voxpupuli/voxpupuli-acceptance/#running-tests
https://voxpupuli.org/docs/how_to_run_tests/
has a bit on running the acceptance tests locally; if you run like this:

BEAKER_destroy=no BEAKER_setfile=debian10-64 bundle exec rake beaker

then you can exec into the docker process and try applying the manifest(s) in /tmp etc. to test / fix testing scenarios locally.

For now, at least, you may need an older Ruby version (not sure if that'll be fixed after modulesync gets updated). Works for me on a machine w/ Ruby 2.6.x

@bastelfreak dropped support for Puppet 6, and I'm working on adding support for newer OS versions (and deprecating old ones) in #928, so you may want to keep an eye on that.

Copy link
Contributor

@wyardley wyardley left a comment

Choose a reason for hiding this comment

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

@kajinamit: Thanks for your hard work on this so far. Looking at this locally, I think the issue is that repos_ensure has to be set to true in data/common.yaml as well. With that updated, the tests pass. I can take a pass at the acceptance tests locally too, but I think we can make a pass at adding this in along with the other breaking changes, possibly 🎉

Let me know if you have time to fix it up; otherwise, I can try to do the remaining work and make a new PR or PR your branch.

We may have some conflicts with some spec related changes in #932 if that's merged first.

But as of now, with the following tweaks (docs fix is just for completeness) for me locally, at least for the unit tests:

--- a/data/common.yaml
+++ b/data/common.yaml
@@ -31,7 +31,7 @@ rabbitmq::repo_gpg_key: ~
 rabbitmq::package_name: 'rabbitmq'
 rabbitmq::package_source: ~
 rabbitmq::package_provider: ~
-rabbitmq::repos_ensure: false
+rabbitmq::repos_ensure: true
 rabbitmq::manage_python: true
 rabbitmq::python_package: 'python'
 rabbitmq::rabbitmq_user: 'rabbitmq'
diff --git a/examples/full.pp b/examples/full.pp
index 9e28ed7..65a9b3a 100644
--- a/examples/full.pp
+++ b/examples/full.pp
@@ -1,6 +1,5 @@
 class { 'rabbitmq':
   delete_guest_user => true,
-  repos_ensure      => true,
   package_apt_pin   => 900,
 }

@@ -15,11 +15,8 @@
context 'default class inclusion' do
let(:pp) do
<<-EOS
class { 'erlang': repo_source => 'packagecloud' } ->
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/voxpupuli/puppet-erlang/blob/master/REFERENCE.md#repo_source This is not valid on all OSes (and I think may be default anyway). I'm testing taking it out and leaving the default here.

@wyardley
Copy link
Contributor

I have the regular specs (the unit tests) for this passing, and conflicts resolved (can make a PR against your branch or send a diff if it helps), but with current versions of stuff getting installed (3.11.x) and current erlang (25.0.x), rabbitmq-server seems to be segfaulting in the acceptance tests. Not sure if this is just that the module doesn't support configs for such a recent version yet, or an issue with the default erlang (though based on the matrix, I think these versions can work together).

Will poke around a little, but we might need to pin to older versions, or hold off on this until we can also set a new minimum supported version of rabbitmq in the module, and adjust the module itself to work with newer rmq.

I'm happy to provide some more guidance - guessing we'll probably get another breaking release out first. The good news is that I think some of these will help make the other work on getting this done a little easier.

@wyardley
Copy link
Contributor

kajinamit#1 has some additional fixes

@kajinamit kajinamit force-pushed the repos-enable branch 2 times, most recently from 5a0bbe7 to ba197d9 Compare May 16, 2023 13:27
@kajinamit
Copy link
Contributor Author

kajinamit commented May 16, 2023

Sorry for my late response. Recent change in my org made it a bit challenging to take time for some work in Puppet modules.

Thanks a lot @wyardley. I've pulled your fix commit into this PR.

@wyardley
Copy link
Contributor

So far, I’m still seeing Erlang segfault on start with the rmq and Erlang version that get pulled down. I tried to get a useful backtrace from the core dump, and couldn’t.

With the rmq version jumping so much, may also be some config stuff to fix. But right now, even just running erl segfaulted for me locally in the beaker test.

kajinamit and others added 3 commits March 28, 2024 18:59
This re-enables repos_ensure by default so that the newer packages
hosted in packagecloud are installed.
Now we install rabbitmq from packagecloud by default. Install erlang
from the same source so that we use the latest versions available for
both rabbitmq and erlang.
@wyardley
Copy link
Contributor

Looks like this is getting closer @kajinamit?

Seems like the debian / ubuntu failures mostly have to do with rabbitmqctl not being present or in the path?

Have you been able to get the acceptance tests working locally?

@wyardley
Copy link
Contributor

Note changes in #974 and if you're able to incorporate #955 (maybe cherry-pick it in), that would be amazing.

Excited that this is hopefully getting closer to shipping.

@wyardley wyardley mentioned this pull request Apr 2, 2024
@wyardley wyardley mentioned this pull request May 18, 2024
@wyardley
Copy link
Contributor

Hi @kajinamit - hopefully some of the recent changes will streamline things a bit and make the tests less brittle if you've got more time to play with this.

Integration tests in Docker are also working for me again, at least for some platforms.

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.

3 participants