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

Bug fixes, lint, rubocop, spec tests etc. #53

Merged
merged 35 commits into from
Sep 29, 2016

Conversation

alexjfisher
Copy link
Member

A couple of bug fixes. Lots of lint fixes. Tests (hardly any) now at least run. See individual commits for details.

alexjfisher and others added 25 commits September 26, 2016 19:55
Update variables to match parameters.
Not just a lint error, an actual bug!

Signed-off-by: Alexander Fisher <[email protected]>
`::include splunk::platform::solaris` had the `::`s in the wrong place.

Signed-off-by: Alexander Fisher <[email protected]>
These were just lint errors as opposed to actual bugs.

Signed-off-by: Alexander Fisher <[email protected]>
* Add `fixtures.yml`.
* Add `default_module_facts.yml` with `staging_http_get` fact.
* Rubocop fixes in `spec/classes/example_spec.rb`.
* Remove completely broken tests.

Looks like there aren't really any tests and
`spec/classes/example_spec.rb` is just boilerplate/skeleton.

For instance there is no `config.pp` manifest, so
```ruby
it { is_expected.to contain_class('splunk::config') }
```
was never going to pass!

Signed-off-by: Alexander Fisher <[email protected]>
Signed-off-by: Alexander Fisher <[email protected]>
For consistency with other Vox Pupuli maintained modules.

Signed-off-by: Alexander Fisher <[email protected]>
Signed-off-by: Alexander Fisher <[email protected]>
Signed-off-by: Alexander Fisher <[email protected]>
Signed-off-by: Alexander Fisher <[email protected]>
Signed-off-by: Alexander Fisher <[email protected]>
@alexjfisher alexjfisher changed the title Bug fixes, lint, spec tests etc. Bug fixes, lint, rubocop, spec tests etc. Sep 26, 2016
@@ -13,7 +13,7 @@ def self.file_path
File.join(@file_path, file_name)
end

def self.set_file_path(path)
def self.set_file_path(path) # rubocop:disable Style/AccessorMethodName
Copy link
Member Author

Choose a reason for hiding this comment

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

I wimped out here. Somebody with more knowledge of the module and how the inifile providers work could have a go if they really wanted to. Probably not worth it though.

Copy link
Contributor

Choose a reason for hiding this comment

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

According to the rubocop docs we should rename this to

def self.file_path=(path)

And then change the references to this method in splunk_config

https://github.com/voxpupuli/puppet-splunk/blob/master/lib/puppet/type/splunk_config.rb#L95-L104

eg;

Puppet::Type.type(res_type).provider(:ini_setting).file_path=(self[:server_confdir])

Copy link
Member Author

Choose a reason for hiding this comment

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

def self.file_path=(path)

leads to Style/TrivialAccessors: Use attr_writer to define trivial writer methods. which can be fixed by replacing the whole thing with

attr_writer :file_path

I think?

Puppet::Type.type(res_type).provider(:ini_setting).file_path=(self[:server_confdir])

leads to Style/SpaceAroundOperators: Surrounding space missing for operator = at which point it was late and I got scared.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you'll need....

class << self
  attr_writer :file_path
end

@alexjfisher
Copy link
Member Author

@crayfishx Would you like to review this one?

@crayfishx crayfishx self-assigned this Sep 27, 2016
Changed set_file_path to be a simple attr_writer.
@crayfishx crayfishx merged commit e861932 into voxpupuli:master Sep 29, 2016
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.

4 participants