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

Fix classes broker::consumer and broker::producer #46

Merged
merged 7 commits into from
Mar 10, 2016
Merged

Fix classes broker::consumer and broker::producer #46

merged 7 commits into from
Mar 10, 2016

Conversation

dhoppe
Copy link
Member

@dhoppe dhoppe commented Mar 10, 2016

I am writing a couple of beaker tests and came across the following problems:

The generated Inits Scripts were broken because of

  • missing equal signs
  • empty parameters
  • obsolete parameters
  • some parameters are required
  • wrong process names

The file resource did not notified the correct service and the config parameters for properties file or Init Script should be managed at the main class.

I know that this would force a major release, but on the other hand this part of the Kafka module could not have worked before.

@bastelfreak
Copy link
Member

could you add a note to the README.md that a major release is required the next time?

@dhoppe
Copy link
Member Author

dhoppe commented Mar 10, 2016

@bastelfreak I will do the next release myself, but I am not sure if I should do this before or after adding the support for Kafka 0.9.

@@ -23,12 +25,12 @@
}

file { '/opt/kafka/config/server.properties':
ensure => present,
Copy link
Contributor

Choose a reason for hiding this comment

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

ensure => 'file'

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not necessary and we do not use it for our other modules.
https://docs.puppetlabs.com/puppet/latest/reference/type.html#file-attribute-ensure

Copy link
Contributor

Choose a reason for hiding this comment

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

i thought puppet-lint would complain…

Copy link
Member Author

Choose a reason for hiding this comment

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

No that was about the broken class mirror. I also should update my RVM environment. ;)

  * Add missing equal sign
  * Fix process name
  * Ignore empty parameters
  * Fix dependencies
  * Fix file permissions
  * Notify correct service
  * Add config, config_defaults
  * Add service_config, service_defaults
@igalic
Copy link
Contributor

igalic commented Mar 10, 2016

nicely done.

are you ready for merge, or do you want to pretty up your commit history?

@bastelfreak
Copy link
Member

IMO the history is fine, nicely seperated small commits

@dhoppe
Copy link
Member Author

dhoppe commented Mar 10, 2016

Give me some time to clear my head. In the meantime I will run some Beaker tests to make sure the configs and init scripts are still generated correctly.

If this is working as expected, I will add the spec files for the Beaker tests as a separate PR.

@igalic
Copy link
Contributor

igalic commented Mar 10, 2016

👍

@bastelfreak
Copy link
Member

👍

bastelfreak added a commit that referenced this pull request Mar 10, 2016
Fix classes broker::consumer and broker::producer
@bastelfreak bastelfreak merged commit 2dff157 into voxpupuli:master Mar 10, 2016
@dhoppe dhoppe deleted the service branch March 10, 2016 14:10
elmendalerenda pushed a commit to elmendalerenda/puppet-kafka that referenced this pull request Mar 30, 2018
Fix classes broker::consumer and broker::producer
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