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

Refactor policy resource #587

Merged
merged 3 commits into from
Apr 6, 2022
Merged

Conversation

fozboz
Copy link
Contributor

@fozboz fozboz commented Apr 6, 2022

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)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (correction or otherwise)
  • Cosmetics (whitespace, appearance)

Checklist

  • I have read the CONTRIBUTING.md document
  • 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.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • 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.

Andy Foster added 2 commits April 6, 2022 10:18
Fixes an idempotency issue where ohai is reloaded every run. The reload
is now triggered only when the `apt_package` resource is updated.
- Modernises LWRP to a custom resource
- Fixes idempotency issue
- Renames `parameters` to `definition` to more acurately reflect
RabbitMQ's language
- Removes `list` action from resource
@fozboz fozboz changed the title Policyfixes Refactor policy resource Apr 6, 2022
@michaelklishin michaelklishin self-assigned this Apr 6, 2022
@michaelklishin michaelklishin merged commit e3e9cec into rabbitmq:v5.x Apr 6, 2022
@michaelklishin
Copy link
Member

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
Copy link
Member

Some test failures with Dokken are due to test-kitchen/dokken-images#57, trying to downgrade Docker Desktop to compare. Welp.

@michaelklishin michaelklishin modified the milestones: 5.9.2, 5.10.0 Apr 7, 2022
@michaelklishin
Copy link
Member

@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
Copy link
Contributor Author

fozboz commented Apr 7, 2022

Unit tests are passing for me now 👍

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
Copy link
Member

michaelklishin commented Apr 7, 2022

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
Copy link
Member

michaelklishin commented Apr 7, 2022

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
Copy link
Contributor Author

fozboz commented Apr 7, 2022

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.

@fozboz fozboz deleted the policyfixes branch April 7, 2022 18:57
@michaelklishin
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants