(PUP-3088) Fix zone provider debug call

[hunner]

The provider should directly call Puppet.debug and Puppet.err
because of the issue described in the PUP ticket.

[joshcooper]

Can you update the commit message with the PUP ticket?

[puppetcla]

CLA signed by all contributors.

[joshcooper]

It looks like there are other calls to self.fail in the provider that will trigger the same bug.

[joshcooper]

The real root cause is that puppet assumes if the resource responds to path then it must respond to tags, file, etc. When that issue is fixed, then this change will cause us to lose context information about the resource that failed, e.g.

Error: /Stage[main]/Main/Zone[test_zone]: Ignoring ...

How about we fix the root cause instead, so we don't lose the context and ensure we work with custom providers that also have a path, e.g. registry_key. Something like?

diff --git a/lib/puppet/util/log.rb b/lib/puppet/util/log.rb
index 08a90b1..8909ab3 100644
--- a/lib/puppet/util/log.rb
+++ b/lib/puppet/util/log.rb
@@ -338,9 +338,9 @@ class Puppet::Util::Log
   def source=(source)
     if source.respond_to?(:path)
       @source = source.path
-      source.tags.each { |t| tag(t) }
-      self.file = source.file
-      self.line = source.line
+      source.tags.each { |t| tag(t) } if source.respond_to?(:tags)
+      self.file = source.file if source.respond_to?(:file)
+      self.line = source.line if source.respond_to?(:line)
     else
       @source = source.to_s
     end

cc'ing @hlindberg and @thallgren who were the last to work on logging messages.

[hunner]

Well, we can't even be sure that the path/tags/file/line methods for providers are the RIGHT methods to call. A developer can make a custom provider with any one of those methods and end up getting unexpected calls, right? It seems like some sort of manifest source callback would be needed that can be referenced instead of the provider methods directly.

[MikaelSmith]

ping @joshcooper what's the state of this?

[MikaelSmith]

I'll follow up on this while @joshcooper's out. @hunner any updates on Josh's comments?

[hunner]

@MikaelSmith Updating the PR now...

[MikaelSmith]

I'm not sure I see the reason in the ticket why this uses the resource, instead of just falling through to to_s.

With the special handling you've added for provider we get
Warning: /File[/var/lib/puppet/rrd]: Fetching file owner.
Letting it fall through (like used to happen for most providers) produced
Warning: File[/var/lib/puppet/rrd](provider=posix): Fetching file owner.

I'd lean towards not changing the behavior for all providers.

[hunner]

Lost comments from Josh on hunner@afdbb23

@MikaelSmith I think you're referring to calling source.path vs. source.resource.path ? This is actually the problem that this commit fixes, as some providers provide .path property getters that override the provider's default path method such as https://github.com/puppetlabs/puppet/blob/3.8.1/lib/puppet/provider/zone/solaris.rb#L64

I can rename the default provider.path method to provider.resource_path or something, though this could have large ramifications than expected if anyone was otherwise depending on this behavior. What would you like?

[MikaelSmith]

No, I mean that for most providers .path wasn't defined, and they would fall through to the else clause to print using to_s. You've changed the behavior on all providers to now use the printing we do for Types, which changes the output as I outlined in my comment.

[MikaelSmith]

👍

[MikaelSmith]

Spec test fails because these aren't invoked.

[kylog]

When would defined?(Puppet::Type) not be true?

[hunner]

That is from the is_resource? method of Puppet::Util::Logging at https://github.com/puppetlabs/puppet/blob/3.8.1/lib/puppet/util/logging.rb#L197-L199 , but this log util may not be used before require 'puppet/type' though the Puppet::Util::Logging one may, but I included it to be safe.

[kylog]

Ah okay; thanks for the context.