(#1532) Replace ParserError with Puppet::Error #1540
Conversation
bastelfreak
requested review from
alexjfisher,
deric,
ekohl,
SimonHoenscheid,
smortex and
a team
as code owners
bastelfreak
changed the title
| @@ -11,6 +11,7 @@ | |||
| allow(provider).to receive(:file_path).and_return('/tmp/foo') | |||
| allow(provider).to receive(:read_file).and_return('foo = bar') | |||
| allow(provider).to receive(:write_file).and_return(true) | |||
| allow(provider).to receive(:resource).and_return(key: 'your_key', line_number: 1, value: 'foo') | |||
There was a problem hiding this comment.
I meant to change line 7 to:
let(:resource) { Puppet::Type.type(:postgresql_conf).new(name: 'foo', key: 'foo', value: 'bar') }There was a problem hiding this comment.
ah okay. will update
There was a problem hiding this comment.
mhm this line is still requires to get the tests passing
There was a problem hiding this comment.
This confuses me and now I wonder what's wrong. Perhaps tomorrow I'll have some idea.
There was a problem hiding this comment.
So I think I know what is going on. Line 7 instantiates a new Puppet type with the desired state. The provider has the actual state. You're mocking that here. So by mocking this, you know it is already in place.
So at the very least I'd expect you to only mock it when you want it to exist, not before each test.
It also makes me wonder about something else. key is a parameter, not a property. So I'm not sure it should be loaded from the provider.
There was a problem hiding this comment.
This line is now redundant, right?
| allow(provider).to receive(:resource).and_return(key: 'your_key', line_number: 1, value: 'foo') |
bastelfreak
force-pushed
the
rubocop2
branch
2 times, most recently
from
7bc3141 to
481eb3f
Compare
bastelfreak
force-pushed
the
rubocop2
branch
from
8638fd9 to
1680e14
Compare
bastelfreak
force-pushed
the
rubocop2
branch
from
1680e14 to
fdf8f8c
Compare
bastelfreak
force-pushed
the
rubocop2
branch
from
fdf8f8c to
5b71fd7
Compare
| @@ -74,7 +74,7 @@ def write_config(file, lines) | |||
| # check, if resource exists in postgresql.conf file | |||
| def exists? | |||
| select = parse_config.select { |hash| hash[:key] == resource[:key] } | |||
| raise ParserError, "found multiple config items of #{resource[:key]} found, please fix this" if select.length > 1 | |||
| raise Puppet::Error, "found multiple config items of #{resource[:key]}, please fix this" if select.length > 1 | |||
There was a problem hiding this comment.
FWIW, providers can call the fail method which will raise Puppet::Error, for example see the base exec provider: https://github.com/puppetlabs/puppet/blob/fce49baa3767fffb000f1b26d3a7b4edbec58999/lib/puppet/provider/exec.rb#L105 There should really be a specification about what methods are available for providers...
There was a problem hiding this comment.
I just wanted to fix a simple typo. I don't know how to properly write tests for the fail() method. I cannot justify to the customer that I spent several days within a 6 month period on this fix. If someone wants to pick it up: feel free.
I'm not sure how we ended up with ParserError in the provider. This exception doesn't exist in Ruby. Puppet ships ther own exception, Puppet::Error. It probably makes sense to raise that instead. Fixes 179472b Alternative implementation for puppetlabs#1538
I'm not sure how we ended up with ParserError in the
provider. This exception doesn't exist in Ruby.
Puppet ships ther own exception, Puppet::Error. It
probably makes sense to raise that instead.
Fixes bastelfreak@179472b
Alternative implementation for #1538
Summary
Provide a detailed description of all the changes present in this pull request.
Additional Context
Add any additional context about the problem here.
Related Issues (if any)
Mention any related issues or pull requests.
Checklist
puppet apply)