Jira (PUP-10105) regression: puppet resource --to_yaml should not emit puppet class tags

0 views
Skip to first unread message

Josh Cooper (JIRA)

unread,
Oct 16, 2019, 12:27:03 PM10/16/19
to puppe...@googlegroups.com
Josh Cooper updated an issue
 
Puppet / Bug PUP-10105
regression: puppet resource --to_yaml should not emit puppet class tags
Change By: Josh Cooper
Summary: regression: puppet resource --to_yaml regression should not emit puppet class tags
Add Comment Add Comment
 
This message was sent by Atlassian JIRA (v7.7.1#77002-sha1:e75ca93)
Atlassian logo

Josh Cooper (JIRA)

unread,
Oct 16, 2019, 1:45:03 PM10/16/19
to puppe...@googlegroups.com
Josh Cooper commented on Bug PUP-10105
 
Re: regression: puppet resource --to_yaml should not emit puppet class tags

This seems to work:

diff --git a/lib/puppet/pops/types/types.rb b/lib/puppet/pops/types/types.rb
index ad0c6b375b..2f29745656 100644
--- a/lib/puppet/pops/types/types.rb
+++ b/lib/puppet/pops/types/types.rb
@@ -1,3 +1,4 @@
+# coding: utf-8
 require_relative 'iterable'
 require_relative 'enumeration'
 require_relative 'recursion_guard'
@@ -736,7 +737,7 @@ class PScalarDataType < PScalarType
   end
 
   def instance?(o, guard = nil)
-    return o.is_a?(String) || o.is_a?(Integer) || o.is_a?(Float) || o.is_a?(TrueClass) || o.is_a?(FalseClass)
+    return o.instance_of?(String) || o.is_a?(Integer) || o.instance_of?(Float) || o.instance_of?(TrueClass) || o.instance_of?(FalseClass)
   end
 
   DEFAULT = PScalarDataType.new

And it warns if the serializer is given a subclass of String (or Float, FalseClass, TrueClass). However, I think we need to keep is_a?(Integer) since it is subclassed by Fixnum and Bignum, and those weren't unified until ruby 2.4.

Also I'd expect instance_of? to be faster since it doesn't need to walk the class hierarchy of the passed in object o.

Henrik Lindberg (JIRA)

unread,
Oct 16, 2019, 2:00:15 PM10/16/19
to puppe...@googlegroups.com

May not be the only place a change is needed since just doing it for Scalar means answers would be inconsistent - i.e. it would say it was a String (puppet data type) unless it also checks with instance_of. Same for the other similar types.

Thomas Hallgren (JIRA)

unread,
Oct 17, 2019, 12:45:03 AM10/17/19
to puppe...@googlegroups.com

I think it makes sense to use instance_of? in all types that describe both Data and RichData. They must not include subclasses of anything. A subclass that wishes to participate in serialization needs to become a puppet type of some sort, typically an Object.

Josh Cooper (JIRA)

unread,
Oct 17, 2019, 1:41:02 AM10/17/19
to puppe...@googlegroups.com
Josh Cooper commented on Bug PUP-10105

Thanks Thomas Hallgren. Couple of follow up questions:

  1. For Data, the PHashType#instance? and PArrayType#instance? methods protect against subclasses, but it looks like PScalarType and PScalarDataType don't, and need to be updated to call instance_of? rather than is_a?.
  2. What about Integer, Fixnum and Bignum? Prior to ruby 2.4, those are different, but related classes. I assume we do want to continuing checking is_a?(Integer) so it matches all 3? Or should Bignum not be an instance of PIntegerType?
  3. For RichData, it looks like most of the instance? methods will need to be updated, e.g. PBinaryType#instance? calls o.is_a?(Binary).
  4. I assume the assignable? logic can continue to use is_a?? For example, a derived String class (like ProcessOutput isn't an instance of PStringType but calling PStringType#assignable? should return true?

Henrik Lindberg (JIRA)

unread,
Oct 17, 2019, 5:59:03 AM10/17/19
to puppe...@googlegroups.com

1. Sounds fine
2. We never supported Bignum so I believe we had is_a?(Fixnum) earlier. Now we have a cap in place on min/max (64 bits) integer value.
3. Sounds right
4. If a class derived from String is not Scalar, then it cannot be allowed to be a String since that would be a paradox. Like Thomas said, it must be its own Object type - this because that is currently the only place in the type system where extensions can be made. So, assignable should also use instance? and return false.

A possible gotcha when doing this is that some tests/mocking may fail since instance?() may not be true for mocks (I think I was bitten by this a couple of times, but don't remember if it is actually true).

Josh Cooper (JIRA)

unread,
Nov 1, 2019, 1:53:03 PM11/1/19
to puppe...@googlegroups.com

Josh Cooper (JIRA)

unread,
Nov 18, 2019, 12:46:04 PM11/18/19
to puppe...@googlegroups.com
Josh Cooper updated an issue
Change By: Josh Cooper
Sprint: Coremunity Grooming Hopper

Rob Braden (JIRA)

unread,
Nov 18, 2019, 1:12:04 PM11/18/19
to puppe...@googlegroups.com
Rob Braden updated an issue
Change By: Rob Braden
Sprint: Platform Core KANBAN Coremunity Hopper

Rob Braden (JIRA)

unread,
Nov 18, 2019, 1:12:05 PM11/18/19
to puppe...@googlegroups.com
Rob Braden updated an issue
Change By: Rob Braden
Sprint: Coremunity Hopper Platform Core KANBAN

Trevor Vaughan (JIRA)

unread,
Dec 23, 2019, 11:58:03 AM12/23/19
to puppe...@googlegroups.com
Trevor Vaughan commented on Bug PUP-10105
 
Re: regression: puppet resource --to_yaml should not emit puppet class tags

This is causing issues with my acceptance tests and breaks YAML.safe_load (which is a best practice). Fundamentally, 99% of the time, I just want the data when using puppet resource.

Example:

require 'yaml'
 
test = %x{puppet resource -y cron}
 
YAML.load(test) # breaks
 
require 'puppet'
 
YAML.safe_load(test) # breaks
 
YAML.load(test) # works, but is unsafe

Josh Cooper (JIRA)

unread,
Jan 16, 2020, 7:58:03 PM1/16/20
to puppe...@googlegroups.com

Austin Boyd (Jira)

unread,
Jul 9, 2021, 8:13:02 AM7/9/21
to puppe...@googlegroups.com
Austin Boyd updated an issue
 
Change By: Austin Boyd
Zendesk Ticket Count: 1
Zendesk Ticket IDs: 45047
This message was sent by Atlassian Jira (v8.13.2#813002-sha1:c495a97)
Atlassian logo

Austin Boyd (Jira)

unread,
Jul 9, 2021, 8:13:02 AM7/9/21
to puppe...@googlegroups.com

Ciprian Badescu (Jira)

unread,
Jul 20, 2021, 10:44:03 AM7/20/21
to puppe...@googlegroups.com

Ciprian Badescu (Jira)

unread,
Jul 20, 2021, 10:44:04 AM7/20/21
to puppe...@googlegroups.com

Ciprian Badescu (Jira)

unread,
Jul 28, 2021, 4:55:03 AM7/28/21
to puppe...@googlegroups.com

Victor Bobosila (Jira)

unread,
Jul 30, 2021, 8:04:02 AM7/30/21
to puppe...@googlegroups.com

Ciprian Badescu (Jira)

unread,
Aug 3, 2021, 6:24:04 AM8/3/21
to puppe...@googlegroups.com

Ciprian Badescu (Jira)

unread,
Aug 11, 2021, 5:21:01 AM8/11/21
to puppe...@googlegroups.com

Gabriel Nagy (Jira)

unread,
Aug 20, 2021, 4:54:02 AM8/20/21
to puppe...@googlegroups.com
Gabriel Nagy commented on Bug PUP-10105
 
Re: regression: puppet resource --to_yaml should not emit puppet class tags

For types subclassing String this was merged in https://github.com/puppetlabs/puppet/commit/bb79ad063382f4055fa77a46039e3441326a82af

I'm not sure if we should handle the other pops changes in this ticket or create another one.

Josh Cooper (Jira)

unread,
Aug 20, 2021, 5:59:03 PM8/20/21
to puppe...@googlegroups.com
Josh Cooper updated an issue
 
Change By: Josh Cooper
Fix Version/s: PUP 7.11.0
Fix Version/s: PUP 6.25.0

Josh Cooper (Jira)

unread,
Aug 20, 2021, 5:59:03 PM8/20/21
to puppe...@googlegroups.com
Josh Cooper commented on Bug PUP-10105
 
Re: regression: puppet resource --to_yaml should not emit puppet class tags

Merged to 6.x in https://github.com/puppetlabs/puppet/commit/bb79ad063382f4055fa77a46039e3441326a82af

Let's file a separate issue for the is_instance?/is_a? issue for Integer/Float/Fixnum/Bignum/Rational

Victor Bobosila (Jira)

unread,
Aug 23, 2021, 9:00:03 AM8/23/21
to puppe...@googlegroups.com

Victor Bobosila (Jira)

unread,
Aug 23, 2021, 9:03:03 AM8/23/21
to puppe...@googlegroups.com
Victor Bobosila updated an issue
Change By: Victor Bobosila
Release Notes Summary: Fix puppet resource --to_yaml emitting puppet class tags (e.g Puppet::Util::Execution::ProcessOutput) by ensuring that PScalarDataType only checks the instance of String, and not other subclasses

Victor Bobosila (Jira)

unread,
Aug 24, 2021, 4:00:03 AM8/24/21
to puppe...@googlegroups.com
Victor Bobosila commented on Bug PUP-10105
 
Re: regression: puppet resource --to_yaml should not emit puppet class tags

Filed a new ticket PUP-11229 for the instance_of?/is_a? issue.

Claire Cadman (Jira)

unread,
Sep 8, 2021, 5:54:02 AM9/8/21
to puppe...@googlegroups.com
Reply all
Reply to author
Forward
0 new messages