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

Sync with pl ops #42

Merged
merged 2 commits into from
May 3, 2012
Merged

Sync with pl ops #42

merged 2 commits into from
May 3, 2012

Conversation

ody
Copy link
Member

@ody ody commented Apr 12, 2012

No description provided.

@reidmv
Copy link

reidmv commented Apr 12, 2012

Couple of comments...

  1. Inclusion of Presets and Separation of Data/Code

    manifests/backports.pp seems to fall under the discussion on http://projects.puppetlabs.com/issues/13273; namely, whether or not to package "preset" apt::source's into the apt module, and if so, how to namespace them. I'm not convinced that including presets in the apt module is desirable, but if done I would prefer to see what is effectively data segregated into a separate namespace, perhaps something like apt::preset::$foo. Failing that, a formal resolution and closure of #13273 would be appreciated. :-)

  2. Clarification of Style Change

    It looks like a large portion of the rest of the PL Ops changes involve setting class-local variables referencing master values in the params.pp file, a la $sources_list_d = $apt::params::sources_list_d. Can someone comment on the advantages to this proposed style switch? In contrast, my personal preference has always been for using fully-qualified names for variables when sourced unchanged from params.pp as this provides a measure of line-level documentation on where a value comes from, when contrasted with non-scoped variables which could previously be assumed to be class-local.

    EDIT: What I'm really looking for with this is for someone to give me something I can use to convince myself that this style is the right way to do things. Right now I'm yet to be convinced, which means that I won't write or update my own modules to use it, which means I don't have consistency in the puppet code I work with, which I'd like to avoid. I like to have consistency wherever possible, and one way of doing that is to use a good style that is generally agreed upon by the community. The closest the current style guide comes to this is the class-parameter-defaults section, which doesn't have a conclusive say on this particular issue.

Good to hear the PL Ops team looking to sync with the publicly published stuff!

@ody
Copy link
Member Author

ody commented Apr 12, 2012

  1. Inclusion of Presets and Separation of Data/Code

Kinda falls under but not quite. They do more than the apt::debian::* things, backports isn't static and goes to the effort of determining some sane defaults for folks that aren't internally mirroring packages.

  1. Clarification of Style Change

Because it makes it so that one follows the same pattern everywhere in the module. For instance, if you want to use class parameter defaults that reference things in a params class you must assign the full qualified to a local variable so might as well do it for non-class parameters too. Plus it cuts down on the amount of stuff I have to type, local variables are a lot shorter than fully qualified. Will also be easier of a pattern to convert when we move to the hiera-next data solution.

@reidmv
Copy link

reidmv commented Apr 13, 2012

  1. Inclusion of Presets and Separation of Data/Code

    I'm still not convinced that backports.pp is significantly different from a preset. The apt module alleges to manage the Advanced Packaging Tool. Repositories that include the concept of releases and backports are logically separate. Marrying apt to the concept of a distribution and by association the concept of a backport is useful, but seems detached from the core purpose of the module as I usually interpret it. Doug McIlroy's "Do one thing and do it well" is the guiding adage I'm trying to reconcile with when I wonder whether such things should be safely segregated away from the core utilities.

  2. Clarification of Style Change

    Ok. That makes sense. I don't care one way or the other about 10 extra characters but if hiera or some other external data source is coming down the road I can see how this would be a useful pattern to be using.

I've been thinking about the title and wording of the merge request and commits. "Syncs up upstream with PL Operations" is not as helpful as it could be to someone reading through the log. It feels like there are really two discrete changes incorporated into the commits, which are

  • Add new defines, apt::backports, apt::conf (this includes new tests)
  • Implement style change with regards to params variables

The fact that these changes were merged into Master from somewhere else is a given and doesn't really need to be the commit short description. I think the commit(s) would be a lot cleaner if they had titles such as the bullets given above and matching long descriptions explaining what was done as per Jeff McCune's commit style suggestions here.

Thoughts?

@ody
Copy link
Member Author

ody commented Apr 20, 2012

  1. Inclusion of Presets and Separation of Data/Code

    I question then where the best place is for defining a set of convenience defines for common tasks? I for one, on every debian machine I use except for the case of needing the super stable gets backports turned on. In my opinion, turning on backports IS a part the generic "apt".

    As for the commit message...I will update it now as I mentioned in IRC the other day.

ody added 2 commits April 20, 2012 13:35
  With the addition of this patch two new defines will be added; one to
  manage APT configuration files (apt::conf) and one that abstracts out the
  requirements needed to turn on backport repositories (apt::backports).

  In addition, the patch takes the opportunity to clean up variable
  definitions so they follow a consistent pattern of setting local
  variables to the fully qualified value stored in the apt::params
  class.  Previously all variable used within a class directly addressed
  the apt::params namespace when ever the variable was used.  In the
  pattern they now adhere to we can more easily switch the namespace
  data lives in or externalize it even more using hiera.
  This patch adds the appropriate spec tests to validate the changes
  introduced by e5f2dfe.  As a bonus it includes fixes to the manifests
  that were discovered while writing the tests.
nanliu added a commit that referenced this pull request May 3, 2012
@nanliu nanliu merged commit 42af4cd into puppetlabs:master May 3, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants