(#11282) PMT creates all five module subdirectories #287

Closed
wants to merge 1 commit into
from

Projects

None yet

7 participants

@ghoneycutt

puppet module generate will now create files, lib, and templates
directories in addition to the manifests, spec, and tests directories
that were already created in order to be complete.

Garrett Honeycutt (#11282) PMT creates all five module subdirectories
puppet module generate will now create files, lib, and templates
directories in addition to the manifests, spec, and tests directories
that were already created in order to be complete.
7d7d5dd
@kelseyhightower

This looks good, I think we may need to update the specs (spec/integration/module_tool_spec.rb) around this behavior.

@kelseyhightower

These readme files are really nice and should be a big help to new users of Puppet, based on the commit message I was expecting code that creates some additional directories, I am missing something?

@ghoneycutt

The code already creates a skeleton directory based on what is in lib/puppet/module_tool/skeleton/templates/generator/

This commit adds the missing directories and README's underneath them. The README's give clarity to what the directories are for as well as get past git not allowing empty directories to be committed.

@ghoneycutt

Not sure what to change in the specs

You can recreate such as

puppet module generate ghoneycutt-foo

notice: Generating module at /Users/gh/git/puppet/pkg/ghoneycutt-foo
ghoneycutt-foo
ghoneycutt-foo/tests
ghoneycutt-foo/tests/init.pp
ghoneycutt-foo/templates
ghoneycutt-foo/templates/README
ghoneycutt-foo/spec
ghoneycutt-foo/spec/spec_helper.rb
ghoneycutt-foo/README
ghoneycutt-foo/Modulefile
ghoneycutt-foo/metadata.json
ghoneycutt-foo/manifests
ghoneycutt-foo/manifests/init.pp
ghoneycutt-foo/lib
ghoneycutt-foo/lib/README
ghoneycutt-foo/files
ghoneycutt-foo/files/README

@kelseyhightower

I will ping one of the Sr, Devs to review this and get this merged.

@jeffmccune

I'm not going to merge this. =)

Whenever I run puppet-module generate I always end up deleting all of those README files out of lib, manifests, files, and templates.

I'd be happy to merge this if all of the documentation were limited to a README.markdown or README.md file in the top level of the generated module.

At a more technical level, we should definitely not put anything in lib/ as this directory is synchronized to agents with pluginsync.

@jeffmccune

Also, to be clear I'm fine creating the directories, I just don't think they should have anything in them by default.

@jeffmccune

Finally, I didn't mean to imply "I'm not going to merge this ever" I just meant "as it is now."

Garrett, what do you think about moving the information into the top level README.md?

@ody
Puppet member

Lets make a final decision on the tests directory rename, especially in light of the existence of the spec directory. And yes I agree with the removal of the README files. Another thing to look at is the exclusion of metadata.json in the initial scaffolding. That file is going to be used more thoroughly by Puppet but needs not to be blank and generated from the contents of the Modulefile which I currently think only happens on a 'puppet module build' task.

@ody
Puppet member

My opinion would be.

puppetlabs-foo
puppetlabs-foo/examples
puppetlabs-foo/examples/init.pp
puppetlabs-foo/templates
puppetlabs-foo/spec
puppetlabs-foo/spec/spec_helper.rb
puppetlabs-foo/README.md
puppetlabs-foo/Modulefile
puppetlabs-foo/manifests
puppetlabs-foo/manifests/init.pp
puppetlabs-foo/lib
puppetlabs-foo/lib/facter
puppetlabs-foo/lib/puppet
puppetlabs-foo/files

Plus extra actions that can generate me server.pp for my foo::server class or a sample template with recommended header. Then a way to easily generate metadata.json.

@jeffmccune

@ody @ghoneycutt

I chatted briefly with @pvande about this and he's fine with renaming tests to examples.

Also, just as an FYI, his "v1" target for the module tool work he's doing is coming up and it doesn't have a ton of module generation and creation stuff in it. We're fine doing merges like this, but he wanted to mention this because the work may not be be re-used by project Geordi.

-Jeff

@ghoneycutt

The README info could certainly be moved to the top level README and out of the individual directories.

They were placed there because git does not allow you to commit empty directories. In light of this, I placed the README's in the directories so I could commit them in version control. If there is another way of doing this, we should do that.

@ghoneycutt

As for renaming tests to examples, that does not have anything to do with this ticket (as the tests directory is already in the code) and should be addressed in a separate ticket. Besides PMT, that would also affect documentation and training.

@willaerk

Hi guys,

just a little observation regarding the README in the toplevel module dir. It seems puppet doc expects the README to contain rdoc markup (in lib/puppet/util/rdoc/parser.rb:132). It will then get rendered along with the rest of the rdoc manifest comments as html.
So, to me it would make sense to rename the README to README.rdoc, to clarify the expectation of puppet doc.

This would also require a change to lib/puppet/util/rdoc/parser.rb, since now it is only looking for a "README".
I have a small patch that I use for this, let me know if I should create a pull request for it.

@ghoneycutt

Willaerk,

Thanks for contributing! Could you create a separate ticket in Redmine[1] and ensure that you have signed our CLA so that we can use your code?

[1] - http://projects.puppetlabs.com/projects/forge

Thanks again,
-g

@willaerk

Ghoneycutt,

signed the CLA, and created a ticket in Redmine (#11801).
I'll create a pull request as soon as possible.

@willaerk

Pull request submitted: 309

@daniel-pittman

@kelseyhightower - you own this; can you make a call on this getting merged or rejected, please, and I will follow that. If you need to coordinate with others, go ahead, but I need a decision from you. :)

@daniel-pittman

@kelseyhightower - any chance your team can review and, if appropriate, merge this, please?

@ghoneycutt

Pinging for update

@cprice404
Puppet member

@kelseyhightower : any news on this one?

@daniel-pittman

@kelseyhightower - can you review this, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment