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

(PUP-5014) Base class for JSON indirection terminus calls #4145

Conversation

tdevelioglu
Copy link

log_exception() wrong.

In Puppet::Indirector::JSON, Puppet.log_exception() is called without
the required exception as the first argument value, instead the message
is passed.

This breaks further along in format_exception() as it attempts to get
the exception message attribute but finds a string:

@kylog
Copy link

kylog commented Aug 10, 2015

@tdevelioglu good catch!

We did a quick audit and it looks like

Puppet.log_exception "Could not save #{self.name} #{request.key}: #{detail}"
suffers from the same problem. (IIRC the msgpack support used the json support as a model, so this omission may have been faithfully reproduced.)

Do you mind extending your patch to include a fix for that one too?

@HAIL9000
Copy link
Contributor

Thanks the controbution @tdevelioglu! In addition to Kylo's comment, would you mind fixing up your commit message a little bit?

It would be a little clearer if you described what the fix is (passing the exception into log_exception) and why the fix was necessary (this broke formatting further down the line and caused puppet to raise an error) rather than just describing the issue.

@tdevelioglu tdevelioglu force-pushed the fix/master/PUP-5014_fix_log_exception_in_json_catalog_terminus branch from 03e7a47 to 7b4ad7a Compare August 11, 2015 09:34
log_exception() wrong

Puppet::Indirector::JSON and Puppet::Indirector::Msgpack call Puppet.log_exception() without
a mandatory exception as the first argument value, passing the message string instead.

This breaks further along in format_exception() when it attempts to get
the exception message attribute but finds a string, resulting in a
NoMethodError raised by the puppet agent.

This commit fixes the issue by passing the raised exception to
log_exception() as required first, and the message second.
@tdevelioglu tdevelioglu force-pushed the fix/master/PUP-5014_fix_log_exception_in_json_catalog_terminus branch from 7b4ad7a to 9d2e0ed Compare August 11, 2015 09:38
@tdevelioglu
Copy link
Author

@HAIL9000
Fix extended to the msgpack base class and updated the commit message.

kylog pushed a commit that referenced this pull request Aug 11, 2015
…_exception_in_json_catalog_terminus

(PUP-5014) Base class for JSON indirection terminus calls
@kylog kylog merged commit 24028d7 into puppetlabs:master Aug 11, 2015
@kylog
Copy link

kylog commented Aug 11, 2015

Thanks again, @tdevelioglu !

@kylog
Copy link

kylog commented Aug 17, 2015

@HAIL9000 in our triage notes, you were going to grep jira for tickets that might map to this. Did you get a chance to do that?

@HAIL9000
Copy link
Contributor

@kylog Not yet! I'll make some time for that this week

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.

3 participants