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 support for cowboy_opts config in rabbitmq.config #1013

Merged
merged 4 commits into from
Sep 6, 2024

Conversation

SpinEternel
Copy link
Contributor

Adds the possibility to use cowboy_opts configuration in the rabbitmq.config.

Unfortunately, i can't test the ssl/cowboy_opts part on my side.

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.

Hi @SpinEternel
Thanks for the contribution!

A few minor comments, also, this should add unit tests for the change you're adding (this will also at least somewhat help with the issue you mentioned around testing the SSL variant).

I believe the tests should be something along the lines of the ones for, e.g., config_kernel_variables.

describe 'config_kernel_variables options' do
let(:params) do
{ config_kernel_variables: {
'inet_dist_listen_min' => 9100,
'inet_dist_listen_max' => 9105
} }
end
it 'sets config variables' do
is_expected.to contain_file('rabbitmq.config'). \
with_content(%r{\{inet_dist_listen_min, 9100\}}). \
with_content(%r{\{inet_dist_listen_max, 9105\}})
end
end

here is a rough example of how you can set other parameters to test different cases.

The regexes or string tests in the tests can be somewhat loose, but should be restrictive enough that they are useful.

manifests/init.pp Outdated Show resolved Hide resolved
templates/rabbitmq.config.epp Show resolved Hide resolved
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.

Also, you'll need the CI checks to pass. For the case of the outdated reference file, you'll need to run the rake task to update the strings. I'd also make sure the other checks (unit tests etc.) pass locally.

manifests/config.pp Outdated Show resolved Hide resolved
@wyardley wyardley added the enhancement New feature or request label Sep 6, 2024
@wyardley
Copy link
Contributor

wyardley commented Sep 6, 2024

See SpinEternel#1

@SpinEternel
Copy link
Contributor Author

Thx a lot for the help ! I didn't have a lot of time to check all the tests. All have passed, sounds good ?

@wyardley wyardley merged commit 47be9d0 into voxpupuli:master Sep 6, 2024
11 checks passed
@wyardley
Copy link
Contributor

wyardley commented Sep 6, 2024

Ack, sorry, meant to squish merge this one.

@wyardley wyardley mentioned this pull request Sep 6, 2024
wyardley added a commit that referenced this pull request Nov 14, 2024
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.

2 participants