Refactor policy resource

fozboz commented 2 years ago

Proposed Changes

This PR contains two commits. The first simply addresses an issue with the ohai resource running on every converge.

The second refactors rabbitmq_policy from an LWRP to a custom resource. This should address #500.

Types of Changes

[ ] Bug fix (non-breaking change which fixes issue #NNNN)
[ ] New feature (non-breaking change which adds functionality)
[x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[x] Documentation (correction or otherwise)
[ ] Cosmetics (whitespace, appearance)

Checklist

[x] I have read the CONTRIBUTING.md document
[x] I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
[ ] All tests pass locally with my changes
- The tests that are failing do not seem to be anything to do with this change. It's probably because I'm running Cinc 17.
[x] I have added tests that prove my fix is effective or that my feature works
[x] I have added necessary documentation (if appropriate)
[x] Any dependent changes have been merged and published in related repositories

Further Comments

I removed the :list action. I didn't see anywhere it was used, and Chef resources should do something, not just return information.

michaelklishin commented 2 years ago

Thank you. The suite needs a revision because CentOS 7 and 8 are basically done. We have switched (some) tests to CentOS 8 Stream but just like every few months, a revision is overdue. I'll try to look into it soon, hopefully Dokken image maintainers are ahead of the curve :)

michaelklishin commented 2 years ago

Some test failures with Dokken are due to https://github.com/test-kitchen/dokken-images/issues/57, trying to downgrade Docker Desktop to compare. Welp.

michaelklishin commented 2 years ago

@fozboz can you please give the tip of v5.x a go and let me know if it looks good enough for a release?

fozboz commented 2 years ago

Unit tests are passing for me now :+1:

Cookstyle still gives me a lot of warnings, and foodcritic has been deprecated. What version of Chef do you want to target? Still >13? That's very old now. Are you happy for me to modernise this as I see fit?

I'm having some issues with integration tests, too. Some examples:

# rabbitmq-erlang-pinned-deb-ubuntu-2004
apt_package[erlang-base] (rabbitmq::erlang_package line 50) had an error: Chef::Exceptions::Package: No candidate version available for erlang-base

# rabbitmq-erlang-pinned-rpm-el8-centos-stream-8
dnf_package[rabbitmq_erlang] (rabbitmq::erlang_package line 62) had an error: Chef::Exceptions::Package: No candidate version available for erlang

# rabbitmq-erlang-latest-rpm-suse-opensuse-leap-15
undefined method `rabbitmq_erlang_zypper_repository_on_suse_factory' for cookbook: rabbitmq, recipe: erlang_package :Chef::Recipe

Before I go digging, can you confirm these suites are working for you?

michaelklishin commented 2 years ago

Yeah, it can be that I messed up the Erlang version numbers for some distributions and installation methods. Erlang Solutions does not provide packages for Debian 11, for example. It also doesn't use the same exact package versioning scheme as our (Team RabbitMQ)'s Erlang packages, so it's an easy mistake to introduce. IIRC ESL packages have a -1 (like Debian package epoch?) at the end.

Amazon Linux 2022 is based on Fedora 34+ but dnf there does not resolve package repos the same way it does on actual Fedora. I am yet to understand why but it's not something about this cookbook.

You are free to look into it, of course. But assuming you run Docker Desktop 4.2.0, most Dokken tests pass. They do for Debian 10, CentOS Stream 8 and Fedora, which is good enough a sanity check if you ask me.

michaelklishin commented 2 years ago

As for the style issues, feel free to address them. I hate FoodCritic with passion, having to update to its rules and rename them is annoying but other contributors saw value in having it, so I am fully on board with keeping and using that suite.

fozboz commented 2 years ago

I've submitted #588 to address linter issues. It looks good for a release to me, but I've also submitted issue #589 to modernise all the LWRPs. That shouldn't take me long, if you want to hold off on the release.

michaelklishin commented 2 years ago

Yup, happy to wait for a few days. Thank you!

rabbitmq / chef-cookbook