Fix puppet-lint issues to raise module's score #10
Conversation
manifests/postgresql_settings.pp
Outdated
| }, | ||
| false => "${::memory['system']['total_bytes'] / 1024 / 1024 / 3}MB", | ||
| true => "${::memory['system']['total_bytes'] / 1024 / 1024 / 8}MB", | ||
| }, |
There was a problem hiding this comment.
Is this really a readability win? I don't think I agree with puppet-lint on this warning
There was a problem hiding this comment.
I'm with you on that. I don't like this change and will put it back.
manifests/postgresql_settings.pp
Outdated
|
|
||
| if ( versioncmp('2017.2.0', $facts['pe_server_version']) >= 0 | ||
| and $manage_fact_values_autovacuum_cost_delay ) { | ||
| if (( versioncmp('2017.2.0', $facts['pe_server_version']) >= 0 ) and $manage_fact_values_autovacuum_cost_delay ) { |
There was a problem hiding this comment.
I also don't think this is a readability win either
manifests/postgresql_settings.pp
Outdated
| pe_databases::set_puppetdb_table_autovacuum_cost_delay_zero { 'reports' : } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
https://docs.puppet.com/puppet/5.0/style_guide.html#spacing-indentation-and-whitespace
Must end the last line with a new line
There was a problem hiding this comment.
interesting. why does that matter? Is this some sort of text editor compatibility thing? Or just arbitrary?
There was a problem hiding this comment.
I'm not actually sure. But I've been putting a new line at the end of manifests ever since I saw that in the style guide.
There was a problem hiding this comment.
I suspect it's like all whitespace rules - there to prevent pointless problems with diffs on merges. It doesn't require an empty blank line, though - if the last line ends with a newline it should be ok, shouldn't it?
manifests/postgresql_settings.pp
Outdated
|
|
||
| if ( versioncmp('2017.2.0', $facts['pe_server_version']) >= 0 | ||
| and $manage_fact_values_autovacuum_cost_delay ) { | ||
| if ( versioncmp('2017.2.0', $facts['pe_server_version']) >= 0 ) |
There was a problem hiding this comment.
What's the complaint here?
There was a problem hiding this comment.
I think between commits you accidentally moved the paran
|
@natemccurdy looks like there's a conflict. If you can fix the paran and the merge conflict I'll look to merge this in. |
|
Most of what this PR tried to change I changed when I made it compatible with PDK. However, I didn't realize you could do multiple line puppet lint ignores so I updated the PR to do that one thing. |
4 participants
Noticed that the module had a low score on the Forge because of these style issues. This PR should help.
There's still one more issue affecting the score, and that's the unbounded dependency on puppetlabs/postgresql. I don't know enough about compatibility to put a limit on the version, so I let that one go.