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

added a "bin_dir" parameter to configure where the Kafka scripts are #159

Merged
merged 4 commits into from
Jul 4, 2017
Merged

Conversation

LionelCons
Copy link
Contributor

Follow-up from PR#147...

@@ -69,6 +72,7 @@
$group_id = $kafka::params::group_id,
$user_id = $kafka::params::user_id,
$config_dir = $kafka::params::config_dir,
$bin_dir = $kafka::params::bin_dir,
Copy link
Member

Choose a reason for hiding this comment

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

@LionelCons can you use the datatype Stdlib::Absolutepath here and in all other locations where you use the new parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bastelfreak It would probably make sense to change all the $*_dir variables at once. Currently, $bin_dir is defined and used exactly like $config_dir.

Copy link
Member

Choose a reason for hiding this comment

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

of course you can change all of them. But I want to have datatypes at least for all new parameters.

@bastelfreak
Copy link
Member

This fixes #145

@bastelfreak
Copy link
Member

This replaces #147

@bastelfreak bastelfreak added the enhancement New feature or request label Jun 29, 2017
@@ -17,6 +17,7 @@
$heap_opts = $kafka::broker::heap_opts,
$opts = $kafka::broker::opts,
$config_dir = $kafka::broker::config_dir,
$bin_dir = $kafka::broker::bin_dir,
Copy link
Member

Choose a reason for hiding this comment

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

we need "Stdlib::Absolutepath " here as well

@@ -18,6 +18,7 @@
$blacklist = $kafka::mirror::blacklist,
$max_heap = $kafka::mirror::max_heap,
$config_dir = $kafka::params::config_dir,
$bin_dir = $kafka::params::bin_dir,
Copy link
Member

@bastelfreak bastelfreak Jun 29, 2017

Choose a reason for hiding this comment

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

we need "Stdlib::Absolutepath " here as well

@@ -12,6 +12,7 @@
$service_defaults = $kafka::consumer::service_defaults,
$service_requires_zookeeper = $kafka::consumer::service_requires_zookeeper,
$limit_nofile = $kafka::consumer::limit_nofile,
$bin_dir = $kafka::consumer::bin_dir,
Copy link
Member

Choose a reason for hiding this comment

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

we need "Stdlib::Absolutepath " here as well

@@ -18,6 +18,7 @@
$scala_version = '2.11'
$install_dir = "/opt/kafka-${scala_version}-${version}"
$config_dir = '/opt/kafka/config'
$bin_dir = '/opt/kafka/bin'
Copy link
Member

Choose a reason for hiding this comment

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

we need "Stdlib::Absolutepath " here as well

Copy link
Member

Choose a reason for hiding this comment

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

ah ups, we don't. all good

@@ -12,6 +12,7 @@
$service_config = $kafka::producer::service_config,
$service_defaults = $kafka::producer::service_defaults,
$service_requires_zookeeper = $kafka::producer::service_requires_zookeeper,
$bin_dir = $kafka::producer::bin_dir,
Copy link
Member

Choose a reason for hiding this comment

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

we need "Stdlib::Absolutepath " here as well

@LionelCons
Copy link
Contributor Author

@bastelfreak I've done for $bin_dir what you have done for the other parameters: define the type only for the public classes and not for the private/internal ones.

Compare for instance broker.pp:

  Boolean $service_install                   = $kafka::params::broker_service_install,
  Enum['running', 'stopped'] $service_ensure = $kafka::params::broker_service_ensure,

with broker/service.pp:

  $service_install            = $kafka::broker::service_install,
  $service_ensure             = $kafka::broker::service_ensure,

@bastelfreak
Copy link
Member

Hi @LionelCons,
we had a discussion on IRC and decided that it is a better approach to add the types to all classes, even the private ones. This ensures consistency. Can you please add the Stdlib::Absolutepath to all occurences of $bin_dir?

@LionelCons
Copy link
Contributor Author

@bastelfreak I've added the type even for private classes.

IMHO, there are solutions to simplify the classes and parameters but let's discuss this elsewhere.

Please accept this PR so that we can move forward.

@bastelfreak bastelfreak merged commit cbdaf85 into voxpupuli:master Jul 4, 2017
@bastelfreak
Copy link
Member

@LionelCons let me know if you need this in a release, we can kick it of pretty quickly.

@LionelCons
Copy link
Contributor Author

@bastelfreak In fact, there are several things that we would need to get fixed/improved. So no need for a new release now...

elmendalerenda pushed a commit to elmendalerenda/puppet-kafka that referenced this pull request Mar 30, 2018
add a "bin_dir" parameter to configure where the Kafka scripts are
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