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

WIP: Fix issues 230, 232, 235, and 249 #250

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nick-markowski
Copy link
Contributor

  • Moved defaults to module data, and removed the params class
  • Privatized all splunk::enterprise:: and splunk::forwarder:: install, config, and service classes
  • Added a $release param, which replaces the $version param
    • For ensurable package_providers, the release is used as the Splunk package ensure, if specified
    • The release no longer defaults to a specific version and build, instead, the Splunk package resource defaults ensure to 'installed'
    • Added a Splunk::Release type
  • Added a service_ensure param, per Add a $service_ensure param to splunk::enterprise and splunk::forwarder #249
  • Modified splunk*_version facts to be part of splunkforwarder and splunkenterprise fact hashes
  • Removed init.pp, which only served to confuse
  • $[enterprise,forwarder]_package_src and $package_source params renamed to $managed_package_source and $unmanaged_package_source, for clarity
  • Fixed: enterprise and forwarder password classes cross-referenced params

Fixed #230
Fixed #232
Fixed #235
Fixed #249

* Moved defaults to module data, and removed the params class
* Privatized all `splunk::enterprise::` and `splunk::forwarder::`
  install, config, and service classes
* Added a `$release` param, which replaces the `$version` param
  * For ensurable package_providers, the release is used as the
    Splunk package ensure, if specified
  * The release no longer defaults to a specific version and build,
    instead, the Splunk package resource defaults ensure to 'installed'
  * Added a Splunk::Release type
* Added a service_ensure param, per voxpupuli#249
* Modified splunk*_version facts to be part of splunkforwarder and
  splunkenterprise fact hashes
* Removed init.pp, which only served to confuse
* `$[enterprise,forwarder]_package_src` and `$package_source` params
  renamed to `$managed_package_source` and `$unmanaged_package_source`,
  for clarity
* Fixed: enterprise and forwarder password classes cross-referenced params

Fixed voxpupuli#230
Fixed voxpupuli#232
Fixed voxpupuli#235
Fixed voxpupuli#249
@nick-markowski nick-markowski changed the title Fix issues 230, 232, 235, and 249 WIP: Fix issues 230, 232, 235, and 249 Jul 31, 2019
@nick-markowski
Copy link
Contributor Author

The bulk of the changes are done, but here are a few notes about the initial commit:

  • This commit finalizes what the original refactor should have been. The module much more closely follows the module best practices, and the refactor was necessary to fix a number of the issues reported by users.
  • Still need to expand upon the README and update REFERENCES.md.
  • I've been experiencing an issue with splunk_config redefining types and throwing errors during puppet-applys, for instance:
  Info: Redefining splunk_distsearch in Puppet::Type
  Error: Could not autoload puppet/type/splunk_config: Attempt to redefine entity 'http://puppet.com/2016.1/runtime/type/splunk_distsearch'. Originally set at file:/etc/puppetlabs/code/modules/splunk/lib/puppet/type/splunk_distsearch.rb?line=3.
  Error: Evaluation Error: Error while evaluating a Resource Statement, Could not autoload puppet/type/splunk_config: Attempt to redefine entity 'http://puppet.com/2016.1/runtime/type/splunk_distsearch'. Originally set at file:/etc/puppetlabs/code/modules/splunk/lib/puppet/type/splunk_distsearch.rb?line=3. (file: /etc/puppetlabs/code/modules/splunk/manifests/enterprise.pp, line: 311, column: 3) on node centos7-64-1.home

String[1] $secret = $splunk::params::secret,
String[1] $splunk_user = $splunk::params::splunk_user,
String[1] $service = $splunk::params::enterprise_service,
Boolean $manage_password = lookup($splunk::enterprise::manage_password),
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add all the lookup() calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After thinking about it, that was silly, and I'm going to remove them

@bastelfreak bastelfreak added backwards-incompatible needs-feedback Further information is requested labels Aug 6, 2019
Copy link
Contributor

@ody ody left a comment

Choose a reason for hiding this comment

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

Overall looks really good. Nice work!

splunk::enterprise::reset_seeded_password: false
splunk::enterprise::secret: 'hhy9DOGqli4.aZWCuGvz8stcqT2/OSJUZuyWHKc4wnJtQ6IZu2bfjeElgYmGHN9RWIT3zs5hRJcX1wGerpMNObWhFue78jZMALs3c3Mzc6CzM98/yGYdfcvWMo1HRdKn82LVeBJI5dNznlZWfzg6xdywWbeUVQZcOZtODi10hdxSJ4I3wmCv0nmkSWMVOEKHxti6QLgjfuj/MOoh8.2pM0/CqF5u6ORAzqFZ8Qf3c27uVEahy7ShxSv2K4K41z'
splunk::enterprise::password_hash: '$6$pIE/xAyP9mvBaewv$4GYFxC0SqonT6/x8qGcZXVCRLUVKODj9drDjdu/JJQ/Iw0Gg.aTkFzCjNAbaK4zcCHbphFz1g1HK18Z2bI92M0'
splunk::enterprise::password_content: ":admin:${password_hash}::Administrator:admin:[email protected]::"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does embedding variables in this way work hiera the same as it did with manifest interpolation? Documentation seems to indicate, no.

https://puppet.com/docs/puppet/latest/hiera_merging.html#the-lookup-and-hiera-function

Copy link
Contributor

Choose a reason for hiding this comment

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

Corroborated by the fact that this was caught in other data files, e.g. Linux.yaml

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, this should certainly be a lookup

setcode do
value = nil
splunk_hash = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I've never encountered another fact that returns an empty hash like this. The behavior I'd expect is that if there is no data related to splunk then the fact should be undefined within a manifest. If the behavior isn't followed then this fact will exist on all nodes within an infrastructure due to the way pluginsync works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I'll make some changes

@nick-markowski
Copy link
Contributor Author

@ody Thank you for the review! I've really dragged my feet digging into this PR after submitting. By now it'll probably need a nasty rebase, but I'll try to get the feedback addressed and get things rolling again.

@pccibot
Copy link

pccibot commented Oct 8, 2019

Dear @nick-markowski, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @nick-markowski, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

1 similar comment
@vox-pupuli-tasks
Copy link

Dear @nick-markowski, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @nick-markowski, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @nick-markowski, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @nick-markowski, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs?
If you need any help, you can reach out to us on our IRC channel voxpupuli on Freenode or our Slack channel voxpupuli at slack.puppet.com.
You can find my sourcecode at voxpupuli/vox-pupuli-tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment