Do you have opinions on what module quality means?

[Ben Ford]

Trick question, I know. Having strong opinions is part of why we do the work we do. So I'm hoping that many of you will help us out on this project.

Some time ago, the Forge turned off the quality scores detail display because many of the warnings were confusing and frightening to less experienced Puppet users. And we didn't really have a good specification of what made quality code anyway. That's what I hope to improve on today, and we'd like to turn it back on with a better picture of each module's quality.

Vox Pupuli has a meta-gem that defines a list of the puppet-lint checks that they use for their definition of a quality Puppet module. I'd like the Forge and Vox Pupuli to be in alignment here, and this seems like the best place to start.

Do you have a favorite list of puppet-lint checks you use? Do you think they'd be useful as an ecosystem standard? Can you support that opinion with a cogent argument for it? Suggest it as a pull request and let's have that conversation. Together as a community we can all come up with a solid starting point as a standard definition of module quality.

https://github.com/voxpupuli/voxpupuli-puppet-lint-plugins/blob/master/voxpupuli-puppet-lint-plugins.gemspec

Thank you all for your help!

--

Ben Ford

@binford2k

Ecosystem Product Manager

[Trevor Vaughan]

I've certainly got a few opinions.

I think that the following are mandatory:

• rspec-puppet tests (that pass)
• beaker, litmus, or test-kitchen tests (that pass)
• Critical path test coverage that matches your supported operating systems (as much as is reasonable)
• Documented parameters (generated docs with puppet-strings should pass)
  
• A reasonable README.md
  
• A generated REFERENCE.md
• A useful Changelog (however you do it)
  
• No embedded binaries
• Use vendor provided packages as a default
• A reasonable update period/response time to bugs
• No telemetry/phone home capabilities
• Fail closed/safe
  

I think that the following are nice to have:

• Arrow alignment
• Indentation
• Muti-node tests (for externally-facing services)

I think that the following are largely pointless:

• Line length checks
• Trailing comma checks
• Code coverage checks
• Taking time to add tests for trivial non-critial-path items just makes it more difficult to refactor the module. Acceptance tests should be used for functionality.

Trevor

_._,_._,_

---

Groups.io Links:

You receive all messages sent to this group.

View/Reply Online (#395) | Reply To Group | Reply To Sender | Mute This Topic | New Topic
Your Subscription | Contact Group Owner | Unsubscribe [tvau...@onyxpoint.com]

_._,_._,_

--

Trevor Vaughan
Vice President, Onyx Point

(410) 541-6699 x788

-- This account not approved for unencrypted proprietary information --

[David Hollinger]

Posting manually since Google Groups doesn't seem to like my ProtonMail E-Mail:

I think we are looking at a couple different discussions here:

• Module Quality
  
• Code Quality
  

I think the former is always going to be difficult to track in any meaningful way, largely because expectations for what a module should or shouldn't do, or even the how it does something, is going to vary from person to person; org to org.

On Module quality, I would say that the Forge should just take a page from the playbook of things like RubyGems, NPMJS, Pypi, etc and just track some combination of:

• Homepage
  
• Downloads
  
• Likes/Stars/Forks on GitHub/Gitlab/BitBucket/etc
  
• Open Issues
  
• List of Files that are installed
  
• List of manual downloads
  
• Release history
  
• Additional useful links
  

On Code Quality, I think the following checks should be done:

• Arrow Alignment
  
• Indentation
  
• Trailing comma (either have or don't have it - standardization is good)
  
• All the checks included in https://github.com/voxpupuli/voxpupuli-puppet-lint-plugins/blob/master/voxpupuli-puppet-lint-plugins.gemspec
  

For tests use badges for the following:

• Unit tests with rspec-puppet 
  
• Acceptance/Integration tests with Beaker/Litmus/test-kitchen
  
• A README
  
• A generated REFERENCE.md from puppet-strings
  
• Changelog that can be embedded into the Forge page
  

Things that I find too subjective to use a markers for quality:

• Use only vendor/OS provided packages:
  While I can see why some would want this, I personally have not worked at a place that could utilize OS provided packages for a lot of non-OS level software. Things like Nginx, Ruby, Go, Apache, PHP, etc tend to be too old even in Ubuntu to use for running applications and in most cases the OS packages/repos have turned into just what is needed to support running applications that contain or install their own requirements - either via containerization or vendor/project provided repositories.
• Don't include binaries:
  This one is, for me, a hard no. With Modules that include Bolt tasks, you limit what kinds of tasks can be included or what languages those tasks can be written in which could be limiting as there are a lot in the Puppet community not interested in learning Ruby, but want their tasks to do work that can't necessarily be done by bash or powershell scripts. So allowing those users to built tasks in Go, Rust, etc is a no brainer, IMO.

Thoughts?

---

David Hollinger III

Software Engineer

WPEngine

Sent with ProtonMail Secure Email.

[Bollinger, John C]

My personal number one measure of module quality is documentation, hands down.  Nothing else comes close.

 

That means a README that provides a good overview of the module’s structure, scope, requirements, and capabilities, and a *thorough* REFERENCE(.md) document.  As a module quality consideration, I don’t really care whether the reference documentation is automatically generated.  I do care that it covers every public member of the module, accurately and precisely explaining the significance, allowed values, default values, and relationships among them.  Strings-derived documentation can be pretty poor, so I don’t attribute much weight to whether docs were generated that way.  If I have to look at manifests or other technical artifacts to determine whether the module supports my needs or how to prod it into doing so then that’s a fail in my book.

 

Good documentary comments are high on my list for code quality, too.  Consistent code style comes in here as well, but not so much most specific style choices.  A good test suite (that passes) also makes the cut, and in it, I would prefer to see both unit tests and functional tests.  I don’t particularly object to the voxpupuli puppet-lint checks, but I consider those a weak proxy for a subjective analysis of the module code.

 

I appreciate that most of that is difficult for a computer to evaluate.

 

 

John Bollinger

 

 

From: voxp...@groups.io <voxp...@groups.io> On Behalf Of David Hollinger via groups.io
Sent: Friday, October 29, 2021 9:44 AM
To: voxp...@groups.io
Cc: puppet...@googlegroups.com
Subject: Re: [voxpupuli] Do you have opinions on what module quality means?

 

Caution: External Sender. Do not open unless you know the content is safe.

 

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

_._,_._,_

---

Groups.io Links:

You receive all messages sent to this group.

View/Reply Online (#398) | Reply To Group | Reply To Sender | Mute This Topic | New Topic
Your Subscription | Contact Group Owner | Unsubscribe [John.Bo...@StJude.org]

_._,_._,_

---

Email Disclaimer: www.stjude.org/emaildisclaimer
Consultation Disclaimer: www.stjude.org/consultationdisclaimer

[Ben Ford]

I want to make sure we don't get too far into the weeds. What we can do for the Forge quality score today is define the metrics correlating to code quality that we can evaluate programmatically. A high quality README is excellent, but not easy for a computer to parse. The best we have is a script that checks for the headers mentioned in the docs page, and that's honestly a terrible approximation.

Right now, what we can realistically evaluate are lint checks and other automated tooling. I'm not a fan of the Forge generating the REFERENCE.md because I also want to read that if I'm reading through the source on GitHub. But what we could do is run puppet-strings and diff the file to validate that it's current. And we could check for the existence of spec files for all classes/defines that don't mark themselves with assert_private(). We could even do more work on my itemize tool to help surface how a module works without forcing you to read source code.

We already have some security lint checks on our roadmap. If you attended Puppetize, you might have heard Florian talk about the project.

I would really love for the Forge to run the spec tests, or somehow validate they're passing. But without a lot more standardization, I don't see how that's possible yet. A lot of people don't even use CI and just manually run their tests before publishing. On a small scale, that's totally fine and I don't think we should dock points for it. Similar thoughts about integration with github/gitlab/bitbucket/etc for things like repository activity.

That doesn't mean that I don't care about the subjective points, just that those will require a different approach. But that's totally a different thread 😂 

---

Email Disclaimer: www.stjude.org/emaildisclaimer
Consultation Disclaimer: www.stjude.org/consultationdisclaimer

_._,_._,_

---

Groups.io Links:

You receive all messages sent to this group.

View/Reply Online (#405) | Reply To Group | Reply To Sender | Mute This Topic | New Topic
Your Subscription | Contact Group Owner | Unsubscribe [ben....@puppet.com]

_._,_._,_

[Ben Ford]

On Thu, Nov 4, 2021 at 8:11 AM Gene Liverman <gene.l...@puppet.com> wrote:

After reading through all the replies, there are bits and pieces from many people that seem to cover most of my opinions. I am going to try and collect those here:

What about this idea? Instead of trying to "measure quality" as a metric, maybe try to expose individual checks or metrics about various aspects of a module and let the user decide "is this high quality for me, or not"?

I like this idea. Present a reasonable set of data about things different users may care about, or might should care about, along with a link to some docs explaining why people care about the listed things.

This makes it real easy for Puppet experts to read, but doesn't do much for new or casual users. This is why we turned the detail view off temporarily on the Forge. Innocuous warnings were unfairly frightening users away from decent modules without high scores. Our roadmap for the rest of the year includes working on a more user friendly view that will reintroduce the details in a more comprehensible way. Some of the score explanations are being driven by this very conversation!

Regarding unit tests, I find the utilization of rspec-puppet-facts (and thereby facterdb) to be a must. I have this opinion for two reasons:

1) as a maintainer, it ensures that my tests work for all the things I have listed in metadata.json (or at least those supported by the gems) which is great in general and especially important when the supported OS lists gets modified.

2) as a user, if I am looking into how a module works it helps me see that the maintainer is testing across all the needed OS's quickly and without having to read every line of every spec test looking for OS combos that I care about.

This is an interesting point. Maybe I'll expand the scope of this just a bit to ask a more meta question. If we're programatically assigning a quality score, do we think that it's a good idea to give points for adhering to a "standard" testing toolchain? eg, puppetlabs_spec_helper, facterdb, pdk, etc.

And if we do, then what about modules in which facterdb doesn't actually provide any benefits? A module that doesn't use facts doesn't need to test with different factsets. How would we distinguish between those cases?

As a user, I want to see that there are at least minimal tests covering public bits - aka at least a "it { is_expected.to compile.with_all_deps }" run via rspec-puppet-facts on each supported os. I prefer to see more but also understand that many people who write puppet code are not nearly as comfortable writing tests.

I'm inclined to say that the bar is that a spec file exists for each manifest. (Ewoud's use case of defining the tests in a single loop instead could be handled by him putting in a file for each path with just a comment explaining where the test actually lives, or something similar.) It would be better to use rspec-puppet test coverage, but I don't think we're ready to actually run the tests on module publication yet. (future improvement?)

 

Regarding integration tests, I love to see them but it takes a lot more knowledge to write them than it does to write a good puppet module. I would love to see straight away that a module has them (and that CI executes them) but wouldn't hold it against an author that they don't have any.

What if the view included a list of platforms the module has acceptance tests for. Informational only, rather than affecting the overall quality score. This would obviously only know the standard testing toolchain(s), of course, but I think this is doable.

Personally, I find having a module documented with puppet-strings to be critical for two reasons:

1) it provides lots of useful information within the source code of the module

2) it enables the programmatic generation of a REFERENCE.md file that can then be read on GitHub/GitLab and rendered on the Forge.

Examples can also be contained within this and there by be referenced by users in either location too. I think README.md should have a very minimal set of examples in it. Most examples should be kept closer to what they are describing via puppet-strings IMO.

Speaking of the README.md, I think looking for select key sections would be worthwhile. I think it should contain the following at a minimum:

- an H1 title at the top

- badges

  - that show the version released on the Forge and link to the module on the Forge

  - build status

  - license (ideally via the shields.io badge that reads the license file)

- an H2 Usage section

- an H2 Reference section that contains at least text referencing REFERENCE.md

- an H2 Changelog section that at least contains text referring to CHANGELOG.md

Sounds like a puppet-readme-lint tool to me! We can improve the spec and test for adherence to it. We could even consider integrating with https://errata-ai.github.io/vale-server/docs/style or some such.

 

One other thing I wish there was a good way to flag on, maybe as part of metadata-json-lint, is when author, summary, license, source, project_page, and issues_url are not filled out in an expected format (or absent all together).

We can absolutely improve metadata-lint to include whatever checks we think are useful. Probably a good first step would be a formal spec for that file 😜

[Ben Ford]

I like the idea of an expand button. We'll probably use something like that. On the other hand, the primary use case of the Forge quality scores is that of a user trying to evaluate and choose modules, so we're highly focused on that. We probably won't confuse it too much with specific details for the author. HOWEVER.....

By end-of-year, I'm writing a score preview extension for the new PDK as a demo of the plugin system so as a module author, you'll get that feedback during the development phase, even before publishing it. Yay, look at that. A plugin system so easy that a product manager can use it, and a tool that gives you early feedback, right when you need it the most!

On Thu, Nov 4, 2021 at 10:14 AM Nick Maludy <nma...@gmail.com> wrote:

Ben,

As you describe the summary scores for new users that makes sense. One UI idea i just had as i read your reply was the following:

- Give an overall summary score "4.3" or something

  - Have a "+" or ">" button that can expand that score and show the components that went into it

  - For each line item show how much each line was worth. This way you can total up the score.

  - Also show the things that were missing and how many points they were worth. This way module developers know what needs to be fixed/improved in order to raise their score.

Just an idea I had and wanted to "brain dump" while it was fresh.

-Nick

_._,_._,_

---

Groups.io Links:

You receive all messages sent to this group.

View/Reply Online (#413) | Reply To Group | Reply To Sender | Mute This Topic | New Topic

Your Subscription | Contact Group Owner | Unsubscribe [ben....@puppet.com]

_._,_._,_

[Trevor Vaughan]

You may want to check CII Best Practices https://bestpractices.coreinfrastructure.org/en/projects/73

On Thu, Nov 4, 2021 at 12:56 PM Ben Ford <ben....@puppet.com> wrote:

_._,_._,_

---

Groups.io Links:

You receive all messages sent to this group.

View/Reply Online (#412) | Reply To Group | Reply To Sender | Mute This Topic | New Topic

Your Subscription | Contact Group Owner | Unsubscribe [tvau...@onyxpoint.com]

_._,_._,_

[Tim Meusel]

Hi,
There were many good ideas on the thread about possible improvements on
the score for modules. Some of them are easy to implement/automate, some
are a bit tricky. I would like to come back to your initial posting Ben.
The goal of voxpupuli-puppet-lint-plugins was to reflect the current
state of best practices and style guides in the Puppet ecosystem. Puppet
has some docs on puppet.com about formatting code. But that does not
contain enough details for newcomers to write good code or to review
pull requests. That's why Vox Pupuli started
https://voxpupuli.org/docs/reviewing_pr/, basically a checklist for code
reviewers that covers the code style.

voxpupuli-puppet-lint-plugins is our solution to automate most of points
from our review checklist. The gem and the website are a living
document, we update it on a regular basis based on discussions within
the community. I think it would be good when not only Vox Pupuli, but
the whole puppet ecosystem tries to follow the same styleguide. That's
why I think the forge should lint modules based on
voxpupuli-puppet-lint-plugins (and that's why I suggested it for
pdk-templates).

Someone doesn't agree with our checklist or some linter plugins in
voxpupuli-puppet-lint-plugins? please raise an issue on:

* https://github.com/voxpupuli/voxpupuli.github.io
* https://github.com/voxpupuli/voxpupuli-puppet-lint-plugins

I believe that a consistent style guide with proper linting is a good
base to onboard newcomers and make their lifes easier but is also the
base for high quality public modules. A proper scoring on the forge
helps us to achieve this and that's important. If that's done there are
many more enhancements for scoring, like the mentioned one in the thread.

Cheers, Tim
Vox Pupuli PMC

On 28.10.21 22:33, Ben Ford wrote:
> Trick question, I know. Having strong opinions is part of why we do the
> work we do. So I'm hoping that many of you will help us out on this project.
>
> Some time ago, the Forge turned off the quality scores detail display
> because many of the warnings were confusing and frightening to less
> experienced Puppet users. And we didn't really have a good specification of
> what made quality code anyway. That's what I hope to improve on today, and
> we'd like to turn it back on with a better picture of each module's quality.
>
> Vox Pupuli has a meta-gem that defines a list of the puppet-lint checks

> <https://github.com/voxpupuli/voxpupuli-puppet-lint-plugins/blob/master/voxpupuli-puppet-lint-plugins.gemspec>