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

Use proper yaml for the mongod config #740

Merged

Conversation

h-haaks
Copy link
Contributor

@h-haaks h-haaks commented Apr 9, 2024

No description provided.

@h-haaks
Copy link
Contributor Author

h-haaks commented Apr 9, 2024

@stevenpost This is just a start with 'systemLog'
Easy to extend to the rest of the config including a merge of @config_data

<% end -%>
<% if @system_logrotate -%>
systemLog.logRotate: <%= @system_logrotate %>
<% config_hash['systemLog']['logRotate'] = @system_logrotate -%>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The naming of existing params is kind of annoying.
Here the param name all of a sudden is prepended with 'system_' ...

<% elsif @verbositylevel == "vvvvv" -%>
systemLog.verbosity: 5
<% config_hash['systemLog']['verbosity'] = 5 -%>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And here class params mimic mongosh -v, -vv options or something? ...

<% if @syslog -%>
systemLog.destination: syslog
<% config_hash['systemLog']['destination'] = 'syslog' -%>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

syslog param sets destination to syslog, but there is no param to set syslogFacility ...

@h-haaks
Copy link
Contributor Author

h-haaks commented Apr 9, 2024

I do think this is the right way to move this.
As we already have a major release going anyways I think we should consider renaming some params and removing seldomly used ones.
I also think expected parameter values should align with https://www.mongodb.com/docs/manual/reference/configuration-options/ and not mongo/mongosh options.

@h-haaks
Copy link
Contributor Author

h-haaks commented Apr 9, 2024

Cleaning up the params of mongodb::server should be done in a separate PR btw.

@h-haaks
Copy link
Contributor Author

h-haaks commented Apr 9, 2024

Requires #741 before converting net. settings to pure yaml.

@stevenpost
Copy link
Contributor

I restored my branch so you can take what you find useful. I think using YAML.safe_load is better suited in unit tests when compared to regexes.

@h-haaks h-haaks force-pushed the real-yaml-in-mongod.conf branch 4 times, most recently from 3a34d30 to 06b713e Compare April 10, 2024 20:58
@h-haaks h-haaks marked this pull request as ready for review April 10, 2024 21:13
@h-haaks h-haaks mentioned this pull request Apr 10, 2024
Comment on lines +116 to +121
<%- if [email protected]? -%>
<%- if !config_hash['net']['http'] -%>
<%- config_hash['net']['http'] = {} -%>
<%- end -%>
<%- config_hash['net']['http']['enabled'] = !@nohttpinterface -%>
<%- end -%>
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably for another PR, but I don't really like these no-prefixed options if the actual option is actually the reverse.

I would keep things more simple by dropping the 'no'.
Like Optional[boolean] $enable_http_interface = undef, but ok, that has nothing to do with this specific PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I know. Just didn't want to drag that into this PR
.

@h-haaks h-haaks merged commit daaa242 into voxpupuli:master Apr 15, 2024
68 checks passed
@h-haaks h-haaks deleted the real-yaml-in-mongod.conf branch April 15, 2024 08:26
@h-haaks h-haaks added the enhancement New feature or request label Apr 28, 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