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 the ability to load definitions on start #472

Merged

Conversation

esabelhaus
Copy link
Contributor

I would like to enable the ability to use the management plugins ability to load definitions.

I've set sane defaults to disable the feature, but it allows someone downstream to create a json file at /etc/rabbitmq/load_definitions.json, and if the attribute is set to true, it would load those definitions defined in the json.

Please let me know if I'm missing anything, and I'll update the PR accordingly.

-Haus

Copy link
Member

@michaelklishin michaelklishin left a comment

Choose a reason for hiding this comment

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

Would it be possible to make the path configurable via node[‘rabbitmq’][‘management’][‘definitions_file’]?

I think a fair number of users would expect this.

@esabelhaus esabelhaus force-pushed the add_management_load_definitions branch from efccf6a to cfa0b17 Compare January 17, 2018 03:35
@esabelhaus
Copy link
Contributor Author

Just did an amend to the commit. Would you like me to document the feature?

@michaelklishin
Copy link
Member

@esabelhaus that would be appreciated. I suggest we add a new README section on definitions. Thanks for the quick turnaround, by the way.

@esabelhaus esabelhaus force-pushed the add_management_load_definitions branch from cfa0b17 to 5e25ef4 Compare January 17, 2018 16:41
@esabelhaus
Copy link
Contributor Author

@michaelklishin added a blurb in the Readme.md, let me know if that will suffice.

@michaelklishin michaelklishin merged commit 5e25ef4 into rabbitmq:master Jan 17, 2018
@michaelklishin
Copy link
Member

Thank you!

@@ -132,6 +132,10 @@
default['rabbitmq']['web_console_ssl'] = false
default['rabbitmq']['web_console_ssl_port'] = 15_671

# If configured to true, allows downstream cookbooks to supply definitions on start
default['rabbitmq']['management']['load_definitions'] = false
default['rabbitmq']['management']['definitions_file'] = '/etc/rabbitmq/load_definitions.json'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest replacing /etc/rabbitmq with node['rabbitmq']['config_root'] for compatibility with different platforms

<% if node['rabbitmq']['management']['load_definitions'] %>
{load_definitions, "<%= node['rabbitmq']['management']['definitions_file'] %>"}
]}
<% end -%>
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it will close the rabbitmq_management config section too early. If you've already tested these changes and it builds a valid config file, feel free to ignore this.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. Should be corrected in 2b6fb6f.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like 2b6fb6f moves the load_definitions section under rabbitmq_management -> listener. Can't wait for the new config file format :)

Copy link
Member

Choose a reason for hiding this comment

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

Corrected again and added the same thing to the non-HTTPS branch. Yeah, the new config format is going to make this stuff drastically simpler.

Copy link
Member

Choose a reason for hiding this comment

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

I'm having trouble with my test kitchen setup, so a test for this may or may not land today but let's see what CI says.

michaelklishin added a commit that referenced this pull request Jan 18, 2018
michaelklishin added a commit that referenced this pull request Jan 18, 2018
This reverts commit 2b6fb6f.

This would be so much less error prone if we only supported 3.7.0.
michaelklishin added a commit that referenced this pull request Jan 18, 2018
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.

3 participants