-
Notifications
You must be signed in to change notification settings - Fork 2.2k
(#21384) Replace current skeleton with a mor extended version #1718
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
Conversation
The current skeleton which is used by ‘puppet module generate’ is very basic and, in my opinion, very out of date. Would like to suggest the skeleton I use for my own puppet modules. Obviously people will have their own opinions but i think it will be a good start for now.
|
CLA Signed by electrical on 2013-06-09 23:31:44 -0700 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this should actually be added by default. Are there sections of the boilerplate that actually require it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The boilerplate has some validation yeah for the current class variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could interfere with Puppet Enterprise as this will force an upgrade of stdlib, and PE users are reluctant to immediately upgrade stdlib for every module if they're already relying on it being a specific version.
|
I think the current module skeleton needs a little freshening, but the extent of this seems a little too opinionated and as likely to fall out of style over time - if not more likely as it adds more decisions. It's already possible to supply your own site skeleton by populating $module_working_dir/skeleton (see https://github.com/garethr/puppet-module-skeleton for a worked example) so I think it'd serve things well to document that mechanism a little more and then allow a thousand site-boilerplate templates to bloom. |
|
I think I'm ok with the skeleton being opinionated, in that it's going to be what users see as they follow learning docs and start creating their own modules. For those new users I think we want to be opinionated and attempt to railroad them in the direction we feel is best for module creation. As they get more experience with Puppet they can come argue the merits of those discussions or ignore them, but I think the skeleton should absolutely evolve with best practices to be opinionated. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be broken by default; if someone tries to add a new file here then they will have to use git add -f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good point. Should use a .gitkeep file i think.
|
@electrical @apenney @hunner have you folks gotten a chance to discuss this more in depth? |
|
We have not. I added a jira ticket to the backlog to act on (FM-29). |
|
Given the google post here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My2 cents:
These default options look generally sane,
I'd leave out $autoupgrade (packages update is generally managed indipendently)
and have some doubt about having separate parameters for $ensure and $version, instead of using directly a single (and simpler to manage) $ensure parameter for both.
I'd add some parameters necessary for reusability.
Some of these may be redundant ( file_content ? the whole dir management? ) and may be evaluated.
class <%= metadata.name %> (
$version = false,
$file_source = undef,
$file_template = undef,
$file_content = undef,
$file_options_hash = undef,
$dir_source = undef,
$dir_purge = false,
$dir_recurse = true,
I think it doesn't make sense to add other "stdmod" parameters here, but it would be nice if puppet module generate could support different templates from which to create the module, this would (maybe already does) allow the usage of different basic skeletons for different modules types.
The implementation details strictly depend on the default parameters, so I'd gladly provide an updated proposal once the default parameters are decided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@electrical @apenney @adrienthebo @rcoleman
I don't know exactly what's the status on this , I'm quite interested in contributing, obviously, and wonder what you think about the proposals in the above comment.
Also, I don't know if there's already a feature like the possibility to choose what skeleton to use for puppet module generate, something like:
puppet module generate --skeleton=...
but I think this would be quite useful if coupled with some other sample skeletons for different module types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've spoken with electrical about this in the past and I think i'm on the side of not merging it until we have the ability to do some kind of puppet module generate profile, where we can have multiple profiles (or skeletons or whatever we call them). I think once that's in place we can drop in several profiles and go from there, rather than trying to pick the perfect set of defaults.
I'm just heading to bed but tomorrow I'll run through this, review it more extensively, and see what we can do in the meantime until we can have multiple skeletons.
|
@apenney @electrical @hunner @rcoleman my understanding is that things are leaning towards a "don't merge" and this has been open for a month; unless someone objects I'm going to close this in the next day or two. |
|
summary: This pull request makes a lot of changes at once, and we haven't been able to come to a consensus for about a month on this. It might be worthwhile to make incremental changes to the skeleton that are easier to merge, instead of an all or nothing pull request that changes a number of things at once. I'm going to go ahead and close this pull request for the time being. Please re-open this pull request once the next actions are addressed, new information is available, or you have a question related to this pull request. We've become aware of difficulties re-opening pull requests, in the event you cannot please mention jeffmccune or adrienthebo with an @ sign in front and we'll re-open this pull request. Closing the pull request doesn't mean we don't consider this change valuable, just that there are things that need to be addressed before it can be merged. If you have any questions or concerns, please don't hesitate to ping us in #puppet-dev on irc.freenode.net. |
The current skeleton which is used by ‘puppet module generate’ is very basic and, in my opinion, very out of date.
Would like to suggest the skeleton I use for my own puppet modules.
Obviously people will have their own opinions but i think it will be a good start for now.