Notes from PR Triage on February 19th, 2014

30 views
Skip to first unread message

Andy Parker

unread,
Feb 19, 2014, 2:55:21 PM2/19/14
to puppe...@googlegroups.com
This time was a little more relax and slow. Lots of things piling up in the backlog of actions… :( We have started punting a lot of these to the 3.6 timeframe. Many are good changes, but with the effort to get 3.5 out we don't want to take on more work right now.

There were several discussions about things that were not directly related to PRs that we triaged. Those have been recorded below.

Joined:

zaphod42 (Andy), nanlie (Nan), hlindberg (Henrik), cyis (Jeremy), kbarber (Ken), joshcooper (Josh), ashp (Ashley), adreyer (Alex), petems (Petems), csharpsteen (Charlie), hunner (Hunter)

Items in strikethrough are closed so can come off the review list next week.
Puppet:
Held over 6x:
2200: It would cause a regression. Contributor pinged. Need some guidance on how to proceed. Peter and Kylo to work on this. Work continues with the discussion around how to handle versions. Kylo will carry the discussion over to puppet-dev. Kylo to continue discussion. Unfortunately we have a decision between regressing and moving to something else that we don't have enough information about versioning on this version of solaris… Nan had some information and options on this. Nan will respond to email with some info.
http://docs.oracle.com/cd/E26502_01/html/E28984/ghyer.html#fmri

Held over 4x:
Puppet:
2086: Ashley and Adrien will work out the details. Looks good. Want to change the provider name from ruby to ini file. Still in progress but might make it in in the coming week.  Merged!!@!!!!!!!!!#@!!!!#@!!@@!##@$!$!@#@!#$!@#$
2276: Looks ok, but we need a little bit of backstory to justify that we can actually do this and needs a ticket. Adrien to look into. Give the contributor another week. Adrien will ping contributor again about info. If we get no response then we will do the investigation ourselves.
Held over 3x:

Puppet:

2247 - looks good but substantial, try to pull into this week's iteration. Got pushed out because of over commitments. Because of the timeline for 3.5 and the number of items left, this is going to have to wait for 3.6 :(

Held over 2x:

Puppet:

1974: We'd like to see this changed to use the package_settings property and clean up the logic (the "permutations" wording is very confusing and not at all clear what it is trying to achieve). Kylo to put in the feedback. Feedback was incomplete. Adrien to flesh out Waiting on contributor. Taking off list for now.

2012: It is doing more than just install options. In fact one of the +1s on the issue wouldn't work because of this. Josh is commenting.  Sent back to contributor because it doesn't get the desired functionality.

2023: Need to squash up and rework the commits. Adrien to take, fix up, to a little validation and merge in (BSD is another area where we'll take what people want) Handing over to Peter Merged
 
2026: Adrien and Ashley to go over it. Ask Charlie take a look. We'll just fix it up time permitting. If anyone else can do it that would make it more likely. Josh will take the one change about the missing method. The rest is fixing the wrong issue. 

2033: We'd love to get this in. Jasperla got stuck on tests and we don't have capacity to get this in right now. Probably in the 3.6 timeframe.

2050: Sending back to Felix to fix up the error messages to be clearer. Joshua to do a small test fixup and merge it in Merged!

2067: Adrien added comment asking about the removing of quoting the value. Felix will try it out on sure box he has. This needs to be pushed to the 3.6 timeframe

Held over 1x:

Puppet:

2348: Some minor fixups. Petems will handle. After some discussion, this will continue in a module

2257: Looks good. Rob merged it in  Merged

2328: Msgpack for on-disk serialization. Henrik merged it in. Merged

2347: Looks good. Charlie will merge it in Merged

2117: Mostly good. Some changes to how it looks up what parser it is using are wanted. Felix will fix up. Andy will merge in. Pushed to 3.6 and made part of PUP-410.

New:

Discussion: default providers in stdlib. Need to be able to specify default providers that are independent of facts. Suggest that we need to improve the power of the default and confine for puppet providers.

2363: HTTPS and FTP support for yumrepo. Henrik will merge it in. Merged

2336: Alex will take a look and comment on what else to do.

2342: Cron….again. Looks ok. Fixes a regression introduced by earlier crontab fixes. Charlie to take a look and merge if good.

2364: After a lot of discussion back and forth, the overall feature doesn't seem to fit well into puppet. Hunter will write up the concerns on the ticket and close the PR.

Discussion: dynamic scoping of resource defaults. Erik would like to see them gone. Felix brings up a discussion that happened in Ghent with Luke and Deepak where the decision was that they are value and widely used on forge modules. Henrik: what is the use case? Erik: the same as dynamic scoping of vars, "send things into classes without specifying parameters". Hunter can't think of a case where he has used resource defaults that wouldn't have adverse with effects with modules being used. Henrik: we are going to have to change all of the internals for puppet 4, so we can address this in puppet 4 and do it one way or the other. Getting rid of dynamic scoping because it is dangerous. Felix thinks that they aren't often used for thinks like File, but are often used with user created defined types.  

--
Andrew Parker
Freenode: zaphod42
Twitter: @aparker42
Software Developer

Join us at PuppetConf 2014, September 23-24 in San Francisco - http://bit.ly/pupconf14

Gary Larizza

unread,
Feb 19, 2014, 3:05:47 PM2/19/14
to puppe...@googlegroups.com
Would like to add support for removing dynamic scoping of resource defaults (and treating them like variable scoping - so local to the namespace). We teaching the concept in the classes we provide, I specifically call this out as a warning (dynamic scoping of defaults) and enforce the concept of specifying them locally when you need them.  I like aiming for the goal of eliminating unexpected behavior in favor of being explicit when you need them.


 
 

--
Andrew Parker
Freenode: zaphod42
Twitter: @aparker42
Software Developer

Join us at PuppetConf 2014, September 23-24 in San Francisco - http://bit.ly/pupconf14

--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to puppet-dev+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CANhgQXtO_Ko_uALc9dfURu_6EMR_9A6BewYZP92gbpRK8YCx%3Dw%40mail.gmail.com.
For more options, visit https://groups.google.com/groups/opt_out.



--
Gary Larizza
Professional Services Engineer
Puppet Labs

Nan Liu

unread,
Feb 19, 2014, 5:14:43 PM2/19/14
to puppet-dev
On Wed, Feb 19, 2014 at 11:55 AM, Andy Parker <an...@puppetlabs.com> wrote:
This time was a little more relax and slow. Lots of things piling up in the backlog of actions… :( We have started punting a lot of these to the 3.6 timeframe. Many are good changes, but with the effort to get 3.5 out we don't want to take on more work right now.

There were several discussions about things that were not directly related to PRs that we triaged. Those have been recorded below.

Joined:

zaphod42 (Andy), nanlie (Nan), hlindberg (Henrik), cyis (Jeremy), kbarber (Ken), joshcooper (Josh), ashp (Ashley), adreyer (Alex), petems (Petems), csharpsteen (Charlie), hunner (Hunter)

Fix, irc: nanliu
 
Items in strikethrough are closed so can come off the review list next week.
Puppet:
Held over 6x:
2200: It would cause a regression. Contributor pinged. Need some guidance on how to proceed. Peter and Kylo to work on this. Work continues with the discussion around how to handle versions. Kylo will carry the discussion over to puppet-dev. Kylo to continue discussion. Unfortunately we have a decision between regressing and moving to something else that we don't have enough information about versioning on this version of solaris… Nan had some information and options on this. Nan will respond to email with some info.
http://docs.oracle.com/cd/E26502_01/html/E28984/ghyer.html#fmri

Commented in PR 2200, basically changing version insync? behavior to regex match leading values seem most appropriate for Solaris 11 pkg (due to noted regression with timestamp). Since we do not want to introduce this behavior to all packages, I suggested implementing insync? in pkg provider, and the type check if the provider respond to insync? and use it as override. 

Ticket created:

This is related to creating a permanent solution to:
https://github.com/puppetlabs/puppetlabs-stdlib/pull/204/

I will make an attempt at a patch, but likely will get into the weeds of type.rb. 

2363: HTTPS and FTP support for yumrepo. Henrik will merge it in. Merged

2336: Alex will take a look and comment on what else to do.

2342: Cron….again. Looks ok. Fixes a regression introduced by earlier crontab fixes. Charlie to take a look and merge if good.

2364: After a lot of discussion back and forth, the overall feature doesn't seem to fit well into puppet. Hunter will write up the concerns on the ticket and close the PR.

Discussion: dynamic scoping of resource defaults. Erik would like to see them gone. Felix brings up a discussion that happened in Ghent with Luke and Deepak where the decision was that they are value and widely used on forge modules. Henrik: what is the use case? Erik: the same as dynamic scoping of vars, "send things into classes without specifying parameters". Hunter can't think of a case where he has used resource defaults that wouldn't have adverse with effects with modules being used. Henrik: we are going to have to change all of the internals for puppet 4, so we can address this in puppet 4 and do it one way or the other. Getting rid of dynamic scoping because it is dangerous. Felix thinks that they aren't often used for thinks like File, but are often used with user created defined types.  

TL;DR, this goes a bit too far trying to 'safe guard' the user.

So one of the suggested workaround to PUP-1738 was to use resource defaults in site.pp. So here's a use case:

File_line {
  provider => ruby,
}

One of the users thread a while back said he would prefer the ability to set resource default behaviors globally, and removing dynamic scoping would eliminate any ability to do this. Starting to feel like I'm always the corner case in these situations :P. 

Thanks,

Nan

Erik Dalén

unread,
Feb 19, 2014, 5:23:31 PM2/19/14
to Puppet Developers
No, removing it means they behave the same as for variables, so they are either global or local. But not passed on through include statements etc. 


--
Erik Dalén

Felix Frank

unread,
Feb 19, 2014, 5:36:22 PM2/19/14
to puppe...@googlegroups.com
On 02/19/2014 11:23 PM, Erik Dalén wrote:
> No, removing it means they behave the same as for variables, so they
> are either global or local. But not passed on through include
> statements etc.

Or even more specifically, variables have three scoping levels IIRC.
1. global scope
2. node scope
3. local scope

And yes, this has proven amply powerful for variables and ought to
suffice for resource defaults as well.

Cheers,
Felix

Nan Liu

unread,
Feb 19, 2014, 5:43:31 PM2/19/14
to puppet-dev
Sorry for missing the G+ discussion, so different behavior whether the resource default is in site.pp, v.s. in node {} v.s. class ... {}?

Just to be clear, I want the following behavior (also isn't this how file backup is configured?):

class { a:
  file_line { 'bla::
  ...
    # provider => ruby implied by site.pp resource defaults.
  } 
}

site.pp: 

File_line {
  provider => ruby,
}

include a

Thanks,

Nan

Erik Dalén

unread,
Feb 19, 2014, 5:48:51 PM2/19/14
to Puppet Developers
Right, that is the intended behaviour.

The behaviour that should be removed is this though:

class b {
  file_line { foo:
    # provider => ruby magically implied by class a resource defaults.
  }
}

class a {
  File_line {
    provider => ruby,
  }
  include b
}


--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to puppet-dev+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CACqVBqBJ%3Dk8Jarqeaxf-CghKy%2BVmSsNgftZpH0cCf%3D%3DECzZuPg%40mail.gmail.com.

For more options, visit https://groups.google.com/groups/opt_out.



--
Erik Dalén

Nan Liu

unread,
Feb 19, 2014, 7:10:41 PM2/19/14
to puppet-dev
On Wed, Feb 19, 2014 at 2:48 PM, Erik Dalén <erik.gus...@gmail.com> wrote:
Right, that is the intended behaviour.

The behaviour that should be removed is this though:

class b {
  file_line { foo:
    # provider => ruby magically implied by class a resource defaults.
  }
}

class a {
  File_line {
    provider => ruby,
  }
  include b
}

Makes sense, thanks for clarifying.

Nan 

Deepak Giridharagopal

unread,
Feb 19, 2014, 9:11:46 PM2/19/14
to puppe...@googlegroups.com
I honestly don't remember the particulars of that conversation...possibly due to the close proximity of belgian beer. :) I don't have an issue with getting rid of dynamically scoped defaults; I've always found them pretty confusing. 
 
Henrik: what is the use case? Erik: the same as dynamic scoping of vars, "send things into classes without specifying parameters". Hunter can't think of a case where he has used resource defaults that wouldn't have adverse with effects with modules being used. Henrik: we are going to have to change all of the internals for puppet 4, so we can address this in puppet 4 and do it one way or the other. Getting rid of dynamic scoping because it is dangerous. Felix thinks that they aren't often used for thinks like File, but are often used with user created defined types.  

--
Andrew Parker
Freenode: zaphod42
Twitter: @aparker42
Software Developer

Join us at PuppetConf 2014, September 23-24 in San Francisco - http://bit.ly/pupconf14

--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to puppet-dev+...@googlegroups.com.

Andy Parker

unread,
Feb 20, 2014, 12:55:48 PM2/20/14
to puppe...@googlegroups.com
On Wed, Feb 19, 2014 at 6:11 PM, Deepak Giridharagopal <dee...@puppetlabs.com> wrote:
On Wed, Feb 19, 2014 at 12:55 PM, Andy Parker <an...@puppetlabs.com> wrote:

Discussion: dynamic scoping of resource defaults. Erik would like to see them gone. Felix brings up a discussion that happened in Ghent with Luke and Deepak where the decision was that they are value and widely used on forge modules.

I honestly don't remember the particulars of that conversation...possibly due to the close proximity of belgian beer. :) I don't have an issue with getting rid of dynamically scoped defaults; I've always found them pretty confusing. 
 

It sounds like there is consensus to get rid of dynamic scoping in all its guises. I'll add that to our list of items for the new evaluator...

:a little later:

It turns out that there was already an issue to deprecate this in 3.6.0 (PUP-867).
 
Henrik: what is the use case? Erik: the same as dynamic scoping of vars, "send things into classes without specifying parameters". Hunter can't think of a case where he has used resource defaults that wouldn't have adverse with effects with modules being used. Henrik: we are going to have to change all of the internals for puppet 4, so we can address this in puppet 4 and do it one way or the other. Getting rid of dynamic scoping because it is dangerous. Felix thinks that they aren't often used for thinks like File, but are often used with user created defined types.  

--
Andrew Parker
Freenode: zaphod42
Twitter: @aparker42
Software Developer

Join us at PuppetConf 2014, September 23-24 in San Francisco - http://bit.ly/pupconf14

--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to puppet-dev+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CANhgQXtO_Ko_uALc9dfURu_6EMR_9A6BewYZP92gbpRK8YCx%3Dw%40mail.gmail.com.

For more options, visit https://groups.google.com/groups/opt_out.

--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to puppet-dev+...@googlegroups.com.

For more options, visit https://groups.google.com/groups/opt_out.
Reply all
Reply to author
Forward
0 new messages