Bug #12377

stdlib facter_dot_d LoadError recovery may be harmful

Added by Jeff McCune 4 months ago. Updated about 1 month ago.

Status:Closed Start date:02/01/2012
Priority:Normal Due date:
Assignee:Ken Barber % Done:

0%
Category:-
Target version:stdlib 2.1.3
Keywords:stdlib Branch:
Votes: 0

Description

Got this email today. Need to look into it soon:

Hi Jeff,

Sorry to bother you, but I believe that this way of recovering from
LoadError can be considered harmful:

      begin
           require 'json'
       rescue LoadError
           require 'rubygems'
           retry
       end

If there is no JSON gem whatsoever, then it will cause an infinite
loop and you will never notice :/

History

#1
Updated by Jeff McCune 4 months ago

  • Status changed from Unreviewed to In Topic Branch Pending Review
  • Assignee set to Jeff McCune

#2
Updated by Jeff McCune 4 months ago

  • Assignee changed from Jeff McCune to Ken Barber

Review please Ken

Ken, would you mind reviewing and merging this if it’s good? It should be a quick one.

Thanks, -Jeff

#3
Updated by Ken Barber 3 months ago

Merged into 2.1.x:

https://github.com/puppetlabs/puppetlabs-stdlib/commit/52bc8809cbdedbacf219289df1f9f48056e6b935

I believe we’ll need to rollup to stdlib 2.2.x for this to work in PE 2.0.x? We probably shouldn’t close this ticket until its done.

Do you want me to do this Jeff? I’m not sure how long its been since we’ve done this last.

#4
Updated by Jeff McCune 3 months ago

On Mon, Feb 6, 2012 at 10:08 AM, tickets@puppetlabs.com wrote:

I believe we’ll need to rollup to 2.2.x for this to work in 2.0.x? We probably shouldn’t close this ticket until its done.

Do you want me to do this Jeff? I’m not sure how long its been since we’ve done this last.

Ken,

The “normal process” to get a change like this into all versions that it should be in would be to merge into 2.0.x to start and then merge up up as 2.0.x => 2.1.x => 2.2.x

You can probably cherry pick the commit easy enough if we want to ship this in 2.0.

We’ll also probably want to ask release engineering to cut new releases of stdlib 2.0, 2.1 and 2.2 as part of the Puppet release cycle next week on 15 February.

Sound good to you? It’d be awesome if you could take care of this, but if you’d rather I do please just let me know and I will.

-Jeff

#5
Updated by Ken Barber 3 months ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release

Jeff McCune wrote:

On Mon, Feb 6, 2012 at 10:08 AM, tickets@puppetlabs.com wrote:

I believe we’ll need to rollup to 2.2.x for this to work in 2.0.x? We probably shouldn’t close this ticket until its done.

Do you want me to do this Jeff? I’m not sure how long its been since we’ve done this last.

Ken,

The “normal process” to get a change like this into all versions that it should be in would be to merge into 2.0.x to start and then merge up up as 2.0.x => 2.1.x => 2.2.x

Yeah I know I’ve been doing it for Facter. BTW there is no 2.0.x branch in stdlib. We’ve committed it to the earliest branch according to the ones on github (which is 2.1.x). So really it needs 2.1.x –> 2.2.x –> master rollup.

You can probably cherry pick the commit easy enough if we want to ship this in 2.0.

We’ll also probably want to ask release engineering to cut new releases of stdlib 2.0, 2.1 and 2.2 as part of the Puppet release cycle next week on 15 February.

Sound good to you? It’d be awesome if you could take care of this, but if you’d rather I do please just let me know and I will.

Sounds good – but like I said I can’t find a 2.0 branch on github for stdlib. Otherwise it sounds fine to line up with the main release calendar.

I’ve just merged this all the way up to master now. Tests succeed – closing ticket. Cheers mate.

#6
Updated by Jeff McCune 3 months ago

Ah, no worries about 2.0.x. I only thought about it because you mentioned, “I believe we’ll need to rollup to 2.2.x for this to work in 2.0.x?”

If there’s no 2.0.x branch, then no need to do anything other than roll up from 2.1.x => 2.2.x => master

-Jeff

#7
Updated by Ken Barber 3 months ago

Yeah – I meant PE 2.0 not stdlib – sorry for the confusion :–).

Jeff McCune wrote:

Ah, no worries about 2.0.x. I only thought about it because you mentioned, “I believe we’ll need to rollup to 2.2.x for this to work in 2.0.x?”

If there’s no 2.0.x branch, then no need to do anything other than roll up from 2.1.x => 2.2.x => master

-Jeff

#8
Updated by Jeff McCune about 1 month ago

  • Project changed from Puppet Enterprise (Public) to Standard Library
  • Category deleted (modules)

#9
Updated by Jeff McCune about 1 month ago

  • Status changed from Merged - Pending Release to Closed
  • Target version changed from PE 2.0.x to stdlib 2.1.3

Released

This bug fix has been released as part of stdlib 2.1.3. It will be merged up into later releases.

Also available in: Atom PDF