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

Add official support for CentOS 8 #900

Closed
wants to merge 2 commits into from

Conversation

kajinamit
Copy link
Contributor

Pull Request (PR) description

This change adds official support for CentOS 8. The current logic is
almost compatible except for the package name of Python 2, which is
python2 in CentOS 8 while python in CentOS 7.

This Pull Request (PR) fixes the following issues

N/A

@kajinamit kajinamit force-pushed the centos-8 branch 6 times, most recently from 1b21ba7 to d865ca9 Compare February 7, 2022 16:20
@kajinamit kajinamit force-pushed the centos-8 branch 2 times, most recently from 52aeea1 to f965a40 Compare February 8, 2022 00:48
repos_ensure = case fact('os.family')
when 'RedHat'
true
end
Copy link
Contributor

Choose a reason for hiding this comment

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

So the current behavior of the module IIRC is that repos_ensure defaults to false. Changing it for the acceptance test here means (I think) that this will now not test the module’s default behavior.

there were some discussions of changing that default again in the module itself, but I think that would be more involved, and that’s where previous attempts at this have kind of stalled in the absence of an active maintainer.

Copy link
Contributor

Choose a reason for hiding this comment

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

#845 has some good discussion and I think is a good starting point.

Copy link
Member

Choose a reason for hiding this comment

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

I think the module should provide sane and working defaults for the different operating systems and the tests should at least cover the defaults. If we test non-default values as well that's a nice addition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand what you mentioned but even for CentOS7 currently this module can't provide complete configuration. This still relies on puppet-eralang to enable epel, now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, AFAIK Ubuntu doesn't require the additional repo and can install rabbitmq from usual OS repos. If we enable repos then default repos will be replaced, which I don't think would be ideal setup.

Copy link
Member

Choose a reason for hiding this comment

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

Why not enable repos_ensure only for RedHat? Since as it seems only RedHat needs extra repos to work out of the box.
That's a common pattern IMO

Copy link
Contributor Author

@kajinamit kajinamit Jul 3, 2022

Choose a reason for hiding this comment

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

This module has never set up the required repositories by default. The current implementation relies on epel repository enabled externally. This change still keeps the same policy and let the users to enable the repository setup by their own(any external modules or setting this parameter.)
I initially hesitated to enable this because it can break existing usage(For example we use this module to deploy rabbitmq as part of OpenStack but in that case we use the rabbitmq package provided by RDO, not packagecloud. Enabling packagecloud repo conflicts with it). However if all agrees we should enable this and make the all users to handle this change properly then I can enable it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kajinamit this module did, at one point, when Puppet still maintained it, setup the RabbitMQ repo by default. It also had an option to include the simple distribution of erlang that rabbitmq uses.

For better or worse, I am responsible for making that change back then (#493). At the time, it seemed like a sane deafult. I think with hindsight, going back to the old behavior (with repos_ensure being default again, as a breaking change) is probably where things should go, it's just a matter of getting there when this project doesn't have an active maintainer.

manifests/init.pp Outdated Show resolved Hide resolved
@bastelfreak bastelfreak added the enhancement New feature or request label Mar 10, 2022
@kajinamit kajinamit force-pushed the centos-8 branch 2 times, most recently from c8fd0de to 926314f Compare March 10, 2022 13:36
@kajinamit
Copy link
Contributor Author

Hmm. Package installation is failing because of missing key. This is likely because two gpg keys are required[1]

[1] https://www.rabbitmq.com/install-rpm.html#package-cloud

@vox-pupuli-tasks
Copy link

Dear @kajinamit, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@kajinamit kajinamit force-pushed the centos-8 branch 2 times, most recently from 9d8c548 to 8cbbe10 Compare July 3, 2022 16:10
@kajinamit kajinamit closed this Aug 16, 2022
@kajinamit kajinamit reopened this Aug 16, 2022
@kajinamit
Copy link
Contributor Author

It seems archi linux job is broken. I see it's failing with the same errors in #924 and I'm not sure how I can fix the job, honestly.

@wyardley
Copy link
Contributor

wyardley commented Mar 8, 2023

It seems archi linux job is broken.

Hi - yes, don't worry about that. It's an issue bootstrapping rabbitmqadmin, and has been there for a while now.

@kajinamit
Copy link
Contributor Author

kajinamit commented Mar 8, 2023

thanks @wyardley .

I've rebased this on the latest master. it seems merge conflict tag is not purged automatically.

I'm wondering what I can do to get this PR merged. This has been floating for a while but I'm really eager to get this(and the subsequent work to support CentOS9) because we are extensively using this for OpenStack deployment.

As I mentioned in the early reply I'm not too sure whether I should enable the repository management which likely cause problems during upgrade. If people think that is the only blocker then I can fix that.

@wyardley
Copy link
Contributor

wyardley commented Mar 8, 2023

@kajinamit maybe worth reading this comment in another PR that I made the other day too
#816 (comment)

High level, while I don't want to bikeshed this to death, this project doesn't have a real active maintainer right now, and so it has been hard to find someone who's able to get some of the things mentioned there working in a safe and testable way.

I do really disagree that we should be installing / managing erlang in this project, at least by default, so at a minimum, we cannot force that behavior without making it configurable and, ideally, set to false by default. That's because people may already have differing needs for which erlang version they need to manage, and / or may already be installing it, and it's important that this module first not break stuff. I'd be Ok with some more junk code to set it up in the integration test, and continue to document that the user needs to manage setting up erlang themself.

I think getting the integration tests running locally would be a good start, if you don't have those working yet; I have had struggles getting that to work recently, but it's honestly going to be very hard to do much on that if you don't have the test suite working (and exec-able into) locally.

You may be able to find some folks from vox who are able to help (esp. w/r/t getting integration tests working properly, as well as some ideas about the best way to handle the erlang dep) on their IRC channel. You could also see if there's any willingness to bump the metadata without appropriate and full working integration tests, but that's not a call I'd make on my own.

@wyardley
Copy link
Contributor

wyardley commented Mar 8, 2023

If https:/voxpupuli/puppet-erlang still is semi-maintained, and works(ish), one option would be to use that in the integration tests if it's not already used there, and just have it be a docs fix or optional dependency for users to use it.

@kajinamit
Copy link
Contributor Author

@wyardley Thanks for these thoughts. these are quite valudable. Do you mind explaining what's the expected coverage of integration tests you mentioned ? I'm asking this because we already have acceptance tests, and I'd like to understand correctly what would be the missing coverage we should fill by integration tests.

While it's difficult to prepare the all supported operating system and versions and test all of these manually, I can prepare c9s box to test this. Also we actually use this module in our own CI so I'll explore the possibility to use that to test this PR.

@wyardley
Copy link
Contributor

wyardley commented Mar 8, 2023

Sorry, I’m using the two terms interchangeably. Acceptance tests.

@kajinamit
Copy link
Contributor Author

If https:/voxpupuli/puppet-erlang still is semi-maintained, and works(ish), one option would be to use that in the integration tests if it's not already used there, and just have it be a docs fix or optional dependency for users to use it.

This is a really good idea, because the module support installing earlang from packagecloud. Let me explore this option.

@kajinamit
Copy link
Contributor Author

ok it seems we have to get the new release of puppet-erlang which contains voxpupuli/puppet-erlang#15 .

@wyardley
Copy link
Contributor

wyardley commented Mar 8, 2023

ok it seems we have to get the new release of puppet-erlang which contains voxpupuli/puppet-erlang#15 .

Sounds good. Key thing is, let's just document it vs. making it a module dependency (since it's not needed for people who bootstrap erlang on their own), or make including it optional vs. some kind of bool, but use it for setting up erlang in the acceptance test if needed.

Other thought: to keep stuff small, maybe we should release the repos_ensure: true breaking change as its own change first (to keep the PRs smaller), and then work on centos 8 / debian 10 etc. support after that?

@bastelfreak
Copy link
Member

The release PR: voxpupuli/puppet-erlang#19

This change makes CentOS 8 and RHEL 8 as a suppotrted OS. For CentOS 8
RabbitMQ package is not available in EPEL and PackageCloud repos should
be used instead.
This change adds support for additional erlang repository so that users
can install rabbitmq correctly without missing dependencies.

Also, now acceptance tests use puppet-erlang instead of  garethr-erlang
so that it can install erlng from packagecloud.

[1] https://www.rabbitmq.com/install-rpm.html
Currently the 'rabbitmq-plguins enable' command does not explicitly
enable the plugin if that is implicitly enabled in CentOS. This change
let the rabbitmq_federation plugin loaded before its management plugin
to workaround the problem.
@kajinamit
Copy link
Contributor Author

Thanks. I just got the job passing. I'll try to find time to attempt to enable repose_ensure by default later this week.

@wyardley
Copy link
Contributor

Seems like CentOS 8 actually was EOL before this PR was even created?
@bastelfreak do you think we should provide support for it in one release, or just close?

@wyardley wyardley closed this May 19, 2024
wyardley added a commit to wyardley/puppet-rabbitmq that referenced this pull request May 20, 2024
Note: CentOS 8 has been EOL for a while, and RHEL 8 is EOL at the end of
May, 2024. Thus, official support may be dropped again soon

Fixes voxpupuli#942
Closes voxpupuli#900
Fixes voxpupuli#816
wyardley added a commit to wyardley/puppet-rabbitmq that referenced this pull request May 20, 2024
Note: CentOS 8 has been EOL for a while, and RHEL 8 is EOL at the end of
May, 2024. Thus, official support may be dropped again soon

Fixes voxpupuli#942
Closes voxpupuli#900
Fixes voxpupuli#816
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants