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

(MODULES-6340) - Address failure when name begins with 9XXX #796

Merged
merged 3 commits into from
Dec 13, 2018

Conversation

eimlav
Copy link
Contributor

@eimlav eimlav commented Dec 11, 2018

When a user specifies a firewall rule starting with a value between 9000-9999, the following error is thrown:

Error: Could not set 'present' on ensure: undefined method `+' for nil:NilClass at /root/manifest.pp:2

The following change addresses this by raising a more appropriate error and providing information in the README as to why this error occurs.

@eimlav eimlav added the bugfix label Dec 11, 2018
@eimlav eimlav changed the title [WIP] MODULES-6340) - Address failure when name begins with 9XXX [WIP] (MODULES-6340) - Address failure when name begins with 9XXX Dec 11, 2018
Copy link
Contributor

@jonnytdevops jonnytdevops left a comment

Choose a reason for hiding this comment

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

I'm not sure I would put this exception inside the provider. The code you've added is checking for nil, and while it is indeed nil for ranges 9000-9999, is may also be nil to as yet unidentified faults.

My recommendation is to:

  • Change the wording to the exception inside the provider to be more generic (e.g. "sorted error detected" or something like that)
  • Add an exception to the type to explicitly disallow a title starting with 9000-9999

Cheers

JT

@eimlav
Copy link
Contributor Author

eimlav commented Dec 12, 2018

@jonnytdevops Initially I had put the exception in the type however the issue I encountered with this is that because the module assigns unmanaged rules a corresponding firewall type including a title with a value in the range 9000-9999 as intended, this will cause any call to retrieve a firewall resource to fail. This is because Puppet encounters these unmanaged resources and matches their title including the 9000-9999 range as being invalid. Perhaps there would need to be an additional property stating whether the resource refers to an unmanaged rule? What are you thoughts on this?

@jonnytdevops
Copy link
Contributor

@eimlav Ah, yes, that is a good point. My suggestion at this point is to somehow tell inside the provider exception if the error is indeed due to a 9000-9999 issue and base the output text on that.

An easier thing you could do is to change the generic error message to provide a indicative advisory (e.g. "Rule sorting error. Make sure that the title of your rule does not start with 9000-9999, as this range is reserved) rather than stating the issue even though we're not sure. It's a "This could be the issue" vs "This is the issue" difference :)

Cheers

JT

@eimlav eimlav changed the title [WIP] (MODULES-6340) - Address failure when name begins with 9XXX (MODULES-6340) - Address failure when name begins with 9XXX Dec 13, 2018
@HelenCampbell HelenCampbell merged commit a6bc5ba into puppetlabs:master Dec 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants