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

Fix fail message #248

Merged
merged 3 commits into from
Mar 7, 2014
Merged

Fix fail message #248

merged 3 commits into from
Mar 7, 2014

Conversation

electrical
Copy link

the fail message was including a fact that was not used in the whole case statement and caused some confusion.

the fail message was including a fact that was not used in the whole case statement and caused some confusion.
@daenney
Copy link

daenney commented Feb 26, 2014

I'm gonna be the annoying community person that's too involved in this but knowing what kind of spec hero you are, could you add a test for that failure message just to prevent someone from bringing the issue back?

@daenney
Copy link

daenney commented Feb 27, 2014

This is good, should merge. @hunner @apenney

@hunner
Copy link

hunner commented Feb 28, 2014

The reason I added $::osfamily even though it's not in the case statement is that if the module is applied to something without $::lsbdistid like Windows (yeah, I know...) then the error message would be less helpful without it.

@electrical
Copy link
Author

@hunner ahh okay. kinda makes sense. will close PR then,

@electrical electrical closed this Feb 28, 2014
@daenney
Copy link

daenney commented Feb 28, 2014

I don't agree with the rationale. @electrical's error message was much better than with the osfamily because the osfamily can obscure the actual error.

Since this module won't ever make any sense on any platform but Debian it should have code in it that bails it out:

if $::osfamily != Debian {
  fail('This module only works on Debian or derivatives like Ubuntu')
}

Now the user knows exactly why it's blowing up and the other error messages in params.pp can reflect the actual cause if it is running on a supported distro.

@hunner
Copy link

hunner commented Feb 28, 2014

++ to that! :) Want to update the PR @electrical ?

@hunner hunner reopened this Feb 28, 2014
@hunner
Copy link

hunner commented Feb 28, 2014

Also, spec/acceptance/unsupported_spec.rb will have to be updated for that error message.

@electrical
Copy link
Author

@hunner will do. Will have a PR after the weekend.

@daenney
Copy link

daenney commented Mar 5, 2014

@electrical Pwetty please? 😄

@electrical
Copy link
Author

Will try to spend some time on it tomorrow :-)

@electrical
Copy link
Author

Working on it today. sorry for the delay

@daenney
Copy link

daenney commented Mar 7, 2014

@hunner Take it away!

hunner added a commit that referenced this pull request Mar 7, 2014
@hunner hunner merged commit 6691c2f into puppetlabs:master Mar 7, 2014
@electrical electrical deleted the fix_params_fail branch March 7, 2014 18:05
@LukasAud LukasAud added the bugfix label Jun 6, 2023
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