Date
1 - 20 of 23
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.
Thank you all for your help!
--
Ben Ford
@binford2k
Ecosystem Product Manager
Ewoud Kohl van Wijngaarden
On Thu, Oct 28, 2021 at 01:33:14PM -0700, Ben Ford wrote:
Personally what I look for are modules that stick close to distribution packages. This not just means installing native OS packages using rpm/deb/... (and not gem/pip/...) but also sticking close to the defaults and a similar configuration layout. Does it use the same file paths, etc. So while you could look at a package resource that specifies a provider, this is really tricky to measure.
What I also look for is simplicity to get started. Perhaps you can measure the number of examples (either in the examples directory or using @example in puppet-strings) but many modules simply have text in the README which is very hard to measure. A big risk of false positives. Also, what is a good example anyway? Do you want to calculate a code-to-example ratio so that a simple module needs fewer examples than a large one?
Clear parameter documentation can only be measured so far. A check like "this @param tag needs to be at least 30 characters" is really bad IMHO. You can put a lot of nonsense in a lot of text while a short text can be very clear.
The best data type is arguable. One thing I generally like is reading Stdlib::Port instead of Integer[0, 65535] but if it describes some configuration setting that describes an unsigned 16-bit integer then it's really the incorrect type. A very strict Pattern[] that exactly matches the data type is also hard to parse for humans. I'm very happy that there can be type aliases so you can read Stdlib::Absolutepath instead of a complex regex. Does that mean you want a limit on the maximum length of a Pattern[] in manifest/**/*.pp and enforce a type alias in types/? What if it's only used once?
One thing that also comes to mind. A good API is hard to misuse. How easy is it to blow up my system/configuration using the module?
Another is how well you stick to semver and easy it to upgrade. Is there a useful changelog? Aren't there too many major version bumps? If I spend more time upgrading the module than actually using it, which problem are you solving?
Security is another. Does it open up my system to security problems out of the box?
So in the end, it comes down to how comfortable the module makes me feel. Do I feel secure in using it?
tl;dr: linting isn't everything ;)
Trick question, I know. Having strong opinions is part of why we do theThis is a terribly complex question. Some quality is measurable, some isn't.
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>
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!
Personally what I look for are modules that stick close to distribution packages. This not just means installing native OS packages using rpm/deb/... (and not gem/pip/...) but also sticking close to the defaults and a similar configuration layout. Does it use the same file paths, etc. So while you could look at a package resource that specifies a provider, this is really tricky to measure.
What I also look for is simplicity to get started. Perhaps you can measure the number of examples (either in the examples directory or using @example in puppet-strings) but many modules simply have text in the README which is very hard to measure. A big risk of false positives. Also, what is a good example anyway? Do you want to calculate a code-to-example ratio so that a simple module needs fewer examples than a large one?
Clear parameter documentation can only be measured so far. A check like "this @param tag needs to be at least 30 characters" is really bad IMHO. You can put a lot of nonsense in a lot of text while a short text can be very clear.
The best data type is arguable. One thing I generally like is reading Stdlib::Port instead of Integer[0, 65535] but if it describes some configuration setting that describes an unsigned 16-bit integer then it's really the incorrect type. A very strict Pattern[] that exactly matches the data type is also hard to parse for humans. I'm very happy that there can be type aliases so you can read Stdlib::Absolutepath instead of a complex regex. Does that mean you want a limit on the maximum length of a Pattern[] in manifest/**/*.pp and enforce a type alias in types/? What if it's only used once?
One thing that also comes to mind. A good API is hard to misuse. How easy is it to blow up my system/configuration using the module?
Another is how well you stick to semver and easy it to upgrade. Is there a useful changelog? Aren't there too many major version bumps? If I spend more time upgrading the module than actually using it, which problem are you solving?
Security is another. Does it open up my system to security problems out of the box?
So in the end, it comes down to how comfortable the module makes me feel. Do I feel secure in using it?
tl;dr: linting isn't everything ;)
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
On Thu, Oct 28, 2021 at 4:33 PM Ben Ford <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 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.Thank you all for your help!--Ben Ford@binford2kEcosystem Product Manager
--
Trevor Vaughan
Vice President, Onyx Point
Vice President, Onyx Point
(410) 541-6699 x788
-- This account not approved for unencrypted proprietary information --
David Hollinger
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?
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, October 28th, 2021 at 3:33 PM, Ben Ford <ben.ford@...> wrote:
On Thursday, October 28th, 2021 at 3:33 PM, Ben Ford <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 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.Thank you all for your help!--Ben Ford@binford2kEcosystem Product Manager
Ewoud Kohl van Wijngaarden
On Fri, Oct 29, 2021 at 08:34:10AM -0400, Trevor Vaughan wrote:
So while this doesn't really affect what the Forge needs to check for directly, it can affect best practices. I'd hate to see it become recommended to store REFERENCE.md in git. Instead, add it to .gitignore and generate it during the release process.
For example, recommend this:
bundle exec rake strings:generate:reference module:push
Personally I don't believe this is within the scope of puppet-blacksmith but it would be good to make some recommendations around this.
I've certainly got a few opinions.Hey, you're just like me :)
- A generated REFERENCE.mdIMHO REFERENCE.md only needs to be present in the actual released tarball. Having it in git is IMHO a bad practice. Generated files do not belong in version control.
So while this doesn't really affect what the Forge needs to check for directly, it can affect best practices. I'd hate to see it become recommended to store REFERENCE.md in git. Instead, add it to .gitignore and generate it during the release process.
For example, recommend this:
bundle exec rake strings:generate:reference module:push
Personally I don't believe this is within the scope of puppet-blacksmith but it would be good to make some recommendations around this.
Trevor Vaughan
REFERENCE.md definitely needs to be in Git.
There are a LOT of places that use Git mirrors and not the forge and the REFERENCE.md is easily readable from pretty much any modern Git interface (and not too bad in vim).
Trevor
On Fri, Oct 29, 2021 at 11:00 AM Ewoud Kohl van Wijngaarden <ewoud+voxpupuli@...> wrote:
On Fri, Oct 29, 2021 at 08:34:10AM -0400, Trevor Vaughan wrote:
>I've certainly got a few opinions.
Hey, you're just like me :)
> - A generated REFERENCE.md
IMHO REFERENCE.md only needs to be present in the actual released
tarball. Having it in git is IMHO a bad practice. Generated files do not
belong in version control.
So while this doesn't really affect what the Forge needs to check for
directly, it can affect best practices. I'd hate to see it become
recommended to store REFERENCE.md in git. Instead, add it to .gitignore
and generate it during the release process.
For example, recommend this:
bundle exec rake strings:generate:reference module:push
Personally I don't believe this is within the scope of puppet-blacksmith
but it would be good to make some recommendations around this.
--
Trevor Vaughan
Vice President, Onyx Point
Vice President, Onyx Point
(410) 541-6699 x788
-- This account not approved for unencrypted proprietary information --
David Hollinger
While there is benefit in the conversation about REFERENCE.md, the purpose of the message chain is specific to what should be displayed on the Forge, should we try to limit the discussion to that for now?
On the topic of the Forge, a REFERENCE.md should definitely be required. I am hesitant to turn scores back on rather than just use download counts, open issues, and age since last release as some sort of general metric.
Aside from badges and files that give the user more information (I.E. Failed tests, what do things do, etc)
Aside from badges and files that give the user more information (I.E. Failed tests, what do things do, etc)
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Friday, October 29th, 2021 at 10:02 AM, Trevor Vaughan <tvaughan@...> wrote:
On Friday, October 29th, 2021 at 10:02 AM, Trevor Vaughan <tvaughan@...> wrote:
REFERENCE.md definitely needs to be in Git.There are a LOT of places that use Git mirrors and not the forge and the REFERENCE.md is easily readable from pretty much any modern Git interface (and not too bad in vim).TrevorOn Fri, Oct 29, 2021 at 11:00 AM Ewoud Kohl van Wijngaarden <ewoud+voxpupuli@...> wrote:On Fri, Oct 29, 2021 at 08:34:10AM -0400, Trevor Vaughan wrote:
>I've certainly got a few opinions.
Hey, you're just like me :)
> - A generated REFERENCE.md
IMHO REFERENCE.md only needs to be present in the actual released
tarball. Having it in git is IMHO a bad practice. Generated files do not
belong in version control.
So while this doesn't really affect what the Forge needs to check for
directly, it can affect best practices. I'd hate to see it become
recommended to store REFERENCE.md in git. Instead, add it to .gitignore
and generate it during the release process.
For example, recommend this:
bundle exec rake strings:generate:reference module:push
Personally I don't believe this is within the scope of puppet-blacksmith
but it would be good to make some recommendations around this.
--Trevor Vaughan
Vice President, Onyx Point(410) 541-6699 x788
-- This account not approved for unencrypted proprietary information --
Ewoud Kohl van Wijngaarden
On Fri, Oct 29, 2021 at 02:43:33PM +0000, David Hollinger wrote:
If we take the definition of quality, then I'd think you want to look at how many bugs does it prevent. IMHO arrow alignment are pure style and you dont catch bugs with it.
Due to large changes in code it does create noise in your git log and thus lower the Maintainability Quality.
Perhaps this is more of a Maintainability Quality?
Pulling an unverified binary from the internet is IMHO very bad, just as using gem/pip/...
However, I do agree that sometime the OS packages aren't an option (too old, not packaged by default). However, often it's possible to use some other repository. That's also useful to know. Also if it manages a repo by default or not.
So not a quality marker per se, but something I always want to know when using a module.
I think we are looking at a couple different discussions here:Completely agreed
- 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:Makes sense.
- 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:I have my doubts about this. I've stopped doing this with class parameters (and defines) and IMHO my code quality improved. On resources it's still standard, but I'm not sure it really matters.
- Arrow Alignment
If we take the definition of quality, then I'd think you want to look at how many bugs does it prevent. IMHO arrow alignment are pure style and you dont catch bugs with it.
Due to large changes in code it does create noise in your git log and thus lower the Maintainability Quality.
- IndentationThis to me is partly style, but I do feel that correct indentation leads to fewer bugs. If you can easily see where the if statement ends, you're less likely to make a mistake. Hugely in favor of this myself.
- Trailing comma (either have or don't have it - standardization is good)This to me is a style thing. Don't get me wrong, I strongly feel that trailing commas are awesome and everyone should use them, you're not likely to catch any bugs with it.
Perhaps this is more of a Maintainability Quality?
- 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:How do you capture decent coverage? IMHO testing private classes directly is often an anti-pattern. Will a single test file give you the positive score?
- Unit tests with rspec-puppet
- Acceptance/Integration tests with Beaker/Litmus/test-kitchenSimilar to the unit tests. Also, do they actually pass? We've seen in Voxpupuli that some modules have them but they don't pass (though I've fought hard to make it easy to run them).
- A READMEThere are many modules out there with a useless README. I am also guilty of this myself.
- A generated REFERENCE.md from puppet-stringsSee my other emails for my thoughts on this.
- Changelog that can be embedded into the Forge pageCertainly agree with this, though maintaining a useful changelog is hard.
Things that I find too subjective to use a markers for quality:I agree these are very subjective. That said, it'd be nice to know how things are installed. My personal policy is to always install using the OS package manager.
- 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.
Pulling an unverified binary from the internet is IMHO very bad, just as using gem/pip/...
However, I do agree that sometime the OS packages aren't an option (too old, not packaged by default). However, often it's possible to use some other repository. That's also useful to know. Also if it manages a repo by default or not.
So not a quality marker per se, but something I always want to know when using a module.
- Don't include binaries:Again, this is something I want to know. As a consumer I want to be able to make an informed decision.
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?Plenty :)
David Hollinger
This to me is a style thing. Don't get me wrong, I strongly feel that trailing commas are awesome and everyone should use them, you're not likely to catch any bugs with it.Definitely a better definition than Code Quality. I think from a community standpoint, these things should be defined clearly by the org/community that is maintaining modules and not by the Forge. But I do think that each community should standardize them.
Perhaps this is more of a Maintainability Quality?
How do you capture decent coverage? IMHO testing private classes directly is often an anti-pattern. Will a single test file give you the positive score?Tests definitely have to be passing and clearly displayed on a badge on the README to be uploaded to the Forge. Now, I'm not sure the Forge should be responsible for validating that tests are passing, but it'd be nice if it could walk the spec directory and see what is being tested and display it somewhere (if that's even possible). Otherwise, I think a badge showing the pipeline/workflow status should be at least the basic requirement.
There are many modules out there with a useless README. I am also guilty of this myself.I'm not sure there's a good way to enforce a "good" README, even if the Forge programatically validated the README layout, it could be easily spoofed. But having one should still be required.
See my other emails for my thoughts on this.
Maybe a better way to put this is some way to validate the puppet-strings are in place for all public parameters and for classes. But a REFERENCE file should be on the Forge regardless of whether it's in Git or not.
Totally agree with this (wherever possible), my only point was that we shouldn't have some requirement about using the OS-maintained packages, just the OS package manager. However, what about OSes that utilize multiple package managers?I agree these are very subjective. That said, it'd be nice to know how things are installed. My personal policy is to always install using the OS package manager.
- Windows - WinGet, MSI
- Fedora - YUM, Flatpak
- Ubuntu - APT, Snap
Either way, there does need to be some sort of clarity about HOW software is being installed by Puppet. Perhaps having separate sub-classes for a package or install class for each separate installation method?
Again, this is something I want to know. As a consumer I want to be able to make an informed decision.My thought here was that the source file should be included in the git repository and that when packaging a puppet module is when the binary should be build for a task. If the task is more than a single go/rust/python file, then perhaps at that point it should be a separate application that the module installs?
Ewoud Kohl van Wijngaarden
On Fri, Oct 29, 2021 at 08:34:31AM -0700, David Hollinger wrote:
* mymodule (so init.pp)
* mymodule::install
* mymodule::config
* mymodule::service
Only mymodule is a public API here so that's likely the only thing being tested. So I would expect a spec/classes/mymodule_spec.rb that has "describe 'mymodule' do" but then resource expectations for all the private classes as well.
I used to write tests that were separated, but in the end I found out that it's very hard to maintain and often leads to slower tests.
I agree that it's out of scope for the Forge. That was also my point. There are things that are module quality in general and module quality that the Forge should care about. IMHO this coding style isn't unless a bad coding style can easily lead to bugs.Definitely a better definition than Code Quality. I think from a community standpoint, these things should be defined clearly by the org/community that is maintaining modules and not by the Forge. But I do think that each community should standardize them.
This to me is a style thing. Don't get me wrong, I strongly feel that
trailing commas are awesome and everyone should use them, you're not
likely to catch any bugs with it.
Perhaps this is more of a Maintainability Quality?
IMHO this is hard to do. Consider a module with only these classes:How do you capture decent coverage? IMHO testing private classes directlyTests definitely have to be passing and clearly displayed on a badge on the README to be uploaded to the Forge. Now, I'm not sure the Forge should be responsible for validating that tests are passing, but it'd be nice if it could walk the spec directory and see what is being tested and display it somewhere (if that's even possible). Otherwise, I think a badge showing the pipeline/workflow status should be at least the basic requirement.
is often an anti-pattern. Will a single test file give you the positive
score?
* mymodule (so init.pp)
* mymodule::install
* mymodule::config
* mymodule::service
Only mymodule is a public API here so that's likely the only thing being tested. So I would expect a spec/classes/mymodule_spec.rb that has "describe 'mymodule' do" but then resource expectations for all the private classes as well.
I used to write tests that were separated, but in the end I found out that it's very hard to maintain and often leads to slower tests.
Agreed.There are many modules out there with a useless README. I am also guiltyI'm not sure there's a good way to enforce a "good" README, even if the Forge programatically validated the README layout, it could be easily spoofed. But having one should still be required.
of this myself.
Agreed. In https://github.com/puppetlabs/puppet-strings/issues/290 we created an issue (based on an earlier conversation). IMHO puppet-strings needs a mode where warnings are fatal so you can include it in your CI as a check. Note that today it can already warn about missing parameter documentation.See my other emails for my thoughts on this.Maybe a better way to put this is some way to validate the puppet-strings are in place for all public parameters and for classes. But a REFERENCE file should be on the Forge regardless of whether it's in Git or not.
Fair point and I did mean OS package manager.I agree these are very subjective. That said, it'd be nice to know howTotally agree with this (wherever possible), my only point was that we shouldn't have some requirement about using the OS-maintained packages, just the OS package manager.
things are installed. My personal policy is to always install using the OS
package manager.
However, what about OSes that utilize multiple package managers?That's also an excellent point that I didn't consider. To me these things are what I expect in a README. We're getting into a best practices, but perhaps we can smooth the path for module authors by providing some good example READMEs that cover these topics? Also how it should be interpreted. In theory it may be something you could capture in metadata.json as well but both suffer from getting out of date compared to the code.
* Windows - WinGet, MSI
* Fedora - YUM, Flatpak
* Ubuntu - APT, Snap
What about 3rd party package managers that are more universal and thus you can only find software via those managers? I.E. Snap, Flatpak, Chocolately, Homebrew, Scoop, etc. I run into this a lot where vendors only provide Snaps or Brew packages even for Linux
Either way, there does need to be some sort of clarity about HOW software is being installed by Puppet. Perhaps having separate sub-classes for a package or install class for each separate installation method?
That sounds fair but then you do get to the point of source module packages and what's extracted. That would tie into the discussion of whether to include Rakefile, spec/* etc or not. Feels like it could be its own discussion.Again, this is something I want to know. As a consumer I want to be ableMy thought here was that the source file should be included in the git repository and that when packaging a puppet module is when the binary should be build for a task. If the task is more than a single go/rust/python file, then perhaps at that point it should be a separate application that the module installs?
to make an informed decision.
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: voxpupuli@groups.io <voxpupuli@groups.io>
On Behalf Of David Hollinger via groups.io
Sent: Friday, October 29, 2021 9:44 AM
To: voxpupuli@groups.io
Cc: puppet-users@...
Subject: Re: [voxpupuli] Do you have opinions on what module quality means?
Email Disclaimer: www.stjude.org/emaildisclaimer
Consultation Disclaimer: www.stjude.org/consultationdisclaimer
Sent: Friday, October 29, 2021 9:44 AM
To: voxpupuli@groups.io
Cc: puppet-users@...
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.
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?
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, October 28th, 2021 at 3:33 PM, Ben Ford <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 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.
Thank you all for your help!
--
Ben Ford
@binford2k
Ecosystem Product Manager
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 😂
On Fri, Oct 29, 2021 at 11:13 AM Bollinger, John C via groups.io <John.Bollinger=StJude.org@groups.io> wrote:
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: voxpupuli@groups.io <voxpupuli@groups.io> On Behalf Of David Hollinger via groups.io
Sent: Friday, October 29, 2021 9:44 AM
To: voxpupuli@groups.io
Cc: puppet-users@...
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.
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?
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, October 28th, 2021 at 3:33 PM, Ben Ford <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 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.
Thank you all for your help!
--
Ben Ford
@binford2k
Ecosystem Product Manager
Email Disclaimer: www.stjude.org/emaildisclaimer
Consultation Disclaimer: www.stjude.org/consultationdisclaimer
Corey Osman
Hi,
toggle quoted message
Show quoted text
One of the items that I like to see in the README is example usage of hiera with the parameters of the classes. Many times I just copy the hiera examples to my internal classes. So I would consider that module to be of high quality because of the hiera documentation. As an example: https://forge.puppet.com/modules/puppet/gitlab
I also want to see tags and versions of the module listed in the project url.
Corey
On Oct 29, 2021, at 7:43 AM, David Hollinger <david.hollinger@...> wrote: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?‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, October 28th, 2021 at 3:33 PM, Ben Ford <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 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.Thank you all for your help!--Ben Ford@binford2kEcosystem Product Manager
Ewoud Kohl van Wijngaarden
On Tue, Nov 02, 2021 at 12:10:03PM -0700, Ben Ford wrote:
As for assert_private(), I must admit that I always try to use
@api private but never assert_private(). As long as the tool can deal with that, it sounds like a decent check.
One thing that I can think of is programatically generating spec tests. In our module we have a lot of trivial classes that are exposing a bunch of plugins. I test them in a loop to avoid a bunch of small files:
https://github.com/theforeman/puppet-foreman/blob/master/spec/classes/cli_plugins_spec.rb
Perhaps a bit of an edge case, but I'd hate to lose score points on this.
rspec-puppet does have coverage reports:
https://rspec-puppet.com/documentation/coverage/
Perhaps that could help? However, it does require running tests which is an area I'd personally like to avoid.
https://github.com/puppetlabs/puppetlabs_spec_helper/blob/a4f45bc4c5c8fabad353550770f98a44acf0d5d7/lib/puppetlabs_spec_helper/rake_tasks.rb#L276-L286
I want to make sure we don't get too far into the weeds. What we can do forThat's I tihnk the point most of us were making: real quality is hard to determine.
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
<https://puppet.com/docs/puppet/7/modules_documentation.html>, and that's
honestly a terrible approximation.
Right now, what we can realistically evaluate are lint checksI would say that you can generally it should be safe to regenerate the reference and diff. There are 2 reasons they can then differ. First is that the developer forgot this when updating their module. The other is that puppet-strings was updated since it was generated (and has changed something in their templates).
<https://github.com/voxpupuli/voxpupuli-puppet-lint-plugins/blob/master/voxpupuli-puppet-lint-plugins.gemspec>
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
<https://github.com/binford2k/binford2k-itemize> to help surface how a
module works without forcing you to read source code.
As for assert_private(), I must admit that I always try to use
@api private but never assert_private(). As long as the tool can deal with that, it sounds like a decent check.
One thing that I can think of is programatically generating spec tests. In our module we have a lot of trivial classes that are exposing a bunch of plugins. I test them in a loop to avoid a bunch of small files:
https://github.com/theforeman/puppet-foreman/blob/master/spec/classes/cli_plugins_spec.rb
Perhaps a bit of an edge case, but I'd hate to lose score points on this.
rspec-puppet does have coverage reports:
https://rspec-puppet.com/documentation/coverage/
Perhaps that could help? However, it does require running tests which is an area I'd personally like to avoid.
We already have some security lint checksIs there a recording of the talk?
<https://github.com/floek/puppet-lint-security-plugins> 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 somehowI always thought it was standardized via puppetlabs_spec_helper and its release_checks task:
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.
https://github.com/puppetlabs/puppetlabs_spec_helper/blob/a4f45bc4c5c8fabad353550770f98a44acf0d5d7/lib/puppetlabs_spec_helper/rake_tasks.rb#L276-L286
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 😂
On Fri, Oct 29, 2021 at 11:13 AM Bollinger, John C via groups.io
<John.Bollinger@...> wrote: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:* voxpupuli@groups.io <voxpupuli@groups.io> *On Behalf Of *David
Hollinger via groups.io
*Sent:* Friday, October 29, 2021 9:44 AM
*To:* voxpupuli@groups.io
*Cc:* puppet-users@...
*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.*
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
<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fvoxpupuli%2Fvoxpupuli-puppet-lint-plugins%2Fblob%2Fmaster%2Fvoxpupuli-puppet-lint-plugins.gemspec&data=04%7C01%7CJohn.Bollinger%40StJude.org%7Ccf737d31ff9549b6860a08d99aea80b7%7C22340fa892264871b677d3b3e377af72%7C0%7C0%7C637711154851057833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=I%2FSmlN1tMXANeU7XowSyYW%2F9Uq5u11Ej7yoD58NwMtA%3D&reserved=0>
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
<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprotonmail.com%2F&data=04%7C01%7CJohn.Bollinger%40StJude.org%7Ccf737d31ff9549b6860a08d99aea80b7%7C22340fa892264871b677d3b3e377af72%7C0%7C0%7C637711154851057833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=KWlq3y32NYPtUbV3ki7Xnva7TeIzylTqUMu%2BfCpsXE4%3D&reserved=0>
Secure Email.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, October 28th, 2021 at 3:33 PM, Ben Ford <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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fvoxpupuli%2Fvoxpupuli-puppet-lint-plugins%2Fblob%2Fmaster%2Fvoxpupuli-puppet-lint-plugins.gemspec&data=04%7C01%7CJohn.Bollinger%40StJude.org%7Ccf737d31ff9549b6860a08d99aea80b7%7C22340fa892264871b677d3b3e377af72%7C0%7C0%7C637711154851067794%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Y%2FlqbI0AptAG66MGn1MITMXJMJzISp7aYiTyBoKohXA%3D&reserved=0>
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
<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fvoxpupuli%2Fvoxpupuli-puppet-lint-plugins%2Fblob%2Fmaster%2Fvoxpupuli-puppet-lint-plugins.gemspec&data=04%7C01%7CJohn.Bollinger%40StJude.org%7Ccf737d31ff9549b6860a08d99aea80b7%7C22340fa892264871b677d3b3e377af72%7C0%7C0%7C637711154851067794%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Y%2FlqbI0AptAG66MGn1MITMXJMJzISp7aYiTyBoKohXA%3D&reserved=0>
Thank you all for your help!
--
Ben Ford
@binford2k
Ecosystem Product Manager
------------------------------
Email Disclaimer: www.stjude.org/emaildisclaimer
Consultation Disclaimer: www.stjude.org/consultationdisclaimer
Nick Maludy
Hey everyone,
I haven't read every single reply here so i apologize if i'm repeating someone else's opinion.
I haven't read every single reply here so i apologize if i'm repeating someone else's opinion.
My general thought about trying to "measure quality" as a single number/metric is going to be difficult. As i've seen from several of the responses, "quality" means something different to each individual person or team.
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'm not trying to create a controversial list here, just some basic stuff that might be helpful. These are things that i had to do when getting a module "approved".
- Does `pdk validate` pass (metadata lint, etc)?
- Does it pass "basic" puppet-lint checks (ones built into PDK for example)?
- Are there unit tests and do they pass?
- Are there integration tests? Do they pass? For what systems/OS?
- Is the module documented with puppet-strings?
- Is the README formatted in the "approved" formatting and sections?
I think green checkmarks or numeric values (depending on the item) would allow the user to make an informed decision.
Thoughts?
-Nick
On Wed, Nov 3, 2021 at 9:24 AM Ewoud Kohl van Wijngaarden <ewoud+voxpupuli@...> wrote:
On Tue, Nov 02, 2021 at 12:10:03PM -0700, Ben Ford wrote:
>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
><https://puppet.com/docs/puppet/7/modules_documentation.html>, and that's
>honestly a terrible approximation.
That's I tihnk the point most of us were making: real quality is hard to
determine.
>Right now, what we can realistically evaluate are lint checks
><https://github.com/voxpupuli/voxpupuli-puppet-lint-plugins/blob/master/voxpupuli-puppet-lint-plugins.gemspec>
>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
><https://github.com/binford2k/binford2k-itemize> to help surface how a
>module works without forcing you to read source code.
I would say that you can generally it should be safe to regenerate the
reference and diff. There are 2 reasons they can then differ. First is
that the developer forgot this when updating their module. The other is
that puppet-strings was updated since it was generated (and has changed
something in their templates).
As for assert_private(), I must admit that I always try to use
@api private but never assert_private(). As long as the tool can deal
with that, it sounds like a decent check.
One thing that I can think of is programatically generating spec tests.
In our module we have a lot of trivial classes that are exposing a bunch
of plugins. I test them in a loop to avoid a bunch of small files:
https://github.com/theforeman/puppet-foreman/blob/master/spec/classes/cli_plugins_spec.rb
Perhaps a bit of an edge case, but I'd hate to lose score points on
this.
rspec-puppet does have coverage reports:
https://rspec-puppet.com/documentation/coverage/
Perhaps that could help? However, it does require running tests which is
an area I'd personally like to avoid.
>We already have some security lint checks
><https://github.com/floek/puppet-lint-security-plugins> on our roadmap. If
>you attended Puppetize, you might have heard Florian talk about the project.
Is there a recording of the talk?
>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.
I always thought it was standardized via puppetlabs_spec_helper and its
release_checks task:
https://github.com/puppetlabs/puppetlabs_spec_helper/blob/a4f45bc4c5c8fabad353550770f98a44acf0d5d7/lib/puppetlabs_spec_helper/rake_tasks.rb#L276-L286
>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 😂
>
>
>
>On Fri, Oct 29, 2021 at 11:13 AM Bollinger, John C via groups.io
><John.Bollinger=StJude.org@groups.io> wrote:
>
>> 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:* voxpupuli@groups.io <voxpupuli@groups.io> *On Behalf Of *David
>> Hollinger via groups.io
>> *Sent:* Friday, October 29, 2021 9:44 AM
>> *To:* voxpupuli@groups.io
>> *Cc:* puppet-users@...
>> *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.*
>>
>>
>>
>> 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
>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fvoxpupuli%2Fvoxpupuli-puppet-lint-plugins%2Fblob%2Fmaster%2Fvoxpupuli-puppet-lint-plugins.gemspec&data=04%7C01%7CJohn.Bollinger%40StJude.org%7Ccf737d31ff9549b6860a08d99aea80b7%7C22340fa892264871b677d3b3e377af72%7C0%7C0%7C637711154851057833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=I%2FSmlN1tMXANeU7XowSyYW%2F9Uq5u11Ej7yoD58NwMtA%3D&reserved=0>
>>
>> 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
>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprotonmail.com%2F&data=04%7C01%7CJohn.Bollinger%40StJude.org%7Ccf737d31ff9549b6860a08d99aea80b7%7C22340fa892264871b677d3b3e377af72%7C0%7C0%7C637711154851057833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=KWlq3y32NYPtUbV3ki7Xnva7TeIzylTqUMu%2BfCpsXE4%3D&reserved=0>
>> Secure Email.
>>
>>
>>
>> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
>> On Thursday, October 28th, 2021 at 3:33 PM, Ben Ford <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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fvoxpupuli%2Fvoxpupuli-puppet-lint-plugins%2Fblob%2Fmaster%2Fvoxpupuli-puppet-lint-plugins.gemspec&data=04%7C01%7CJohn.Bollinger%40StJude.org%7Ccf737d31ff9549b6860a08d99aea80b7%7C22340fa892264871b677d3b3e377af72%7C0%7C0%7C637711154851067794%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Y%2FlqbI0AptAG66MGn1MITMXJMJzISp7aYiTyBoKohXA%3D&reserved=0>
>> 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
>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fvoxpupuli%2Fvoxpupuli-puppet-lint-plugins%2Fblob%2Fmaster%2Fvoxpupuli-puppet-lint-plugins.gemspec&data=04%7C01%7CJohn.Bollinger%40StJude.org%7Ccf737d31ff9549b6860a08d99aea80b7%7C22340fa892264871b677d3b3e377af72%7C0%7C0%7C637711154851067794%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Y%2FlqbI0AptAG66MGn1MITMXJMJzISp7aYiTyBoKohXA%3D&reserved=0>
>>
>>
>>
>> Thank you all for your help!
>>
>>
>>
>> --
>>
>> Ben Ford
>>
>> @binford2k
>>
>> Ecosystem Product Manager
>>
>>
>>
>>
>> ------------------------------
>>
>> Email Disclaimer: www.stjude.org/emaildisclaimer
>> Consultation Disclaimer: www.stjude.org/consultationdisclaimer
>>
>>
>>
>
>
>
>
>
Martin Alfke
Hi,
to be honest: I really liked that there was the possibility for user to provide individual feedback on forge.
When it comes to quality, I see the following topics:
- pdk validate passing (without disabling any check globally)
- acceptance tests
- strings documentation
- reference.md as part of the release (also in GIT - many people want to read the reference even when not having strings installed)
- supported puppet versions (older ones are OK, but stable and supported puppet versions should be covered
Additionally I see the following topics covering "active community" question:
- response time on issues (not fixing, but when will the issue author receive the first feedback/information)
- regular releases (minimum bi-yearly or more often)
Best,
Martin
toggle quoted message
Show quoted text
to be honest: I really liked that there was the possibility for user to provide individual feedback on forge.
When it comes to quality, I see the following topics:
- pdk validate passing (without disabling any check globally)
- acceptance tests
- strings documentation
- reference.md as part of the release (also in GIT - many people want to read the reference even when not having strings installed)
- supported puppet versions (older ones are OK, but stable and supported puppet versions should be covered
Additionally I see the following topics covering "active community" question:
- response time on issues (not fixing, but when will the issue author receive the first feedback/information)
- regular releases (minimum bi-yearly or more often)
Best,
Martin
On 3. Nov 2021, at 14:57, Nick Maludy <nmaludy@...> wrote:
Hey everyone,
I haven't read every single reply here so i apologize if i'm repeating someone else's opinion.
My general thought about trying to "measure quality" as a single number/metric is going to be difficult. As i've seen from several of the responses, "quality" means something different to each individual person or team.
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'm not trying to create a controversial list here, just some basic stuff that might be helpful. These are things that i had to do when getting a module "approved".
- Does `pdk validate` pass (metadata lint, etc)?
- Does it pass "basic" puppet-lint checks (ones built into PDK for example)?
- Are there unit tests and do they pass?
- Are there integration tests? Do they pass? For what systems/OS?
- Is the module documented with puppet-strings?
- Is the README formatted in the "approved" formatting and sections?
I think green checkmarks or numeric values (depending on the item) would allow the user to make an informed decision.
Thoughts?
-Nick
On Wed, Nov 3, 2021 at 9:24 AM Ewoud Kohl van Wijngaarden <ewoud+voxpupuli@...> wrote:
On Tue, Nov 02, 2021 at 12:10:03PM -0700, Ben Ford wrote:I want to make sure we don't get too far into the weeds. What we can do forThat's I tihnk the point most of us were making: real quality is hard to
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
<https://puppet.com/docs/puppet/7/modules_documentation.html>, and that's
honestly a terrible approximation.
determine.Right now, what we can realistically evaluate are lint checksI would say that you can generally it should be safe to regenerate the
<https://github.com/voxpupuli/voxpupuli-puppet-lint-plugins/blob/master/voxpupuli-puppet-lint-plugins.gemspec>
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
<https://github.com/binford2k/binford2k-itemize> to help surface how a
module works without forcing you to read source code.
reference and diff. There are 2 reasons they can then differ. First is
that the developer forgot this when updating their module. The other is
that puppet-strings was updated since it was generated (and has changed
something in their templates).
As for assert_private(), I must admit that I always try to use
@api private but never assert_private(). As long as the tool can deal
with that, it sounds like a decent check.
One thing that I can think of is programatically generating spec tests.
In our module we have a lot of trivial classes that are exposing a bunch
of plugins. I test them in a loop to avoid a bunch of small files:
https://github.com/theforeman/puppet-foreman/blob/master/spec/classes/cli_plugins_spec.rb
Perhaps a bit of an edge case, but I'd hate to lose score points on
this.
rspec-puppet does have coverage reports:
https://rspec-puppet.com/documentation/coverage/
Perhaps that could help? However, it does require running tests which is
an area I'd personally like to avoid.We already have some security lint checksIs there a recording of the talk?
<https://github.com/floek/puppet-lint-security-plugins> 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 somehowI always thought it was standardized via puppetlabs_spec_helper and its
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.
release_checks task:
https://github.com/puppetlabs/puppetlabs_spec_helper/blob/a4f45bc4c5c8fabad353550770f98a44acf0d5d7/lib/puppetlabs_spec_helper/rake_tasks.rb#L276-L286That 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 😂
On Fri, Oct 29, 2021 at 11:13 AM Bollinger, John C via groups.io
<John.Bollinger@...> wrote: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:* voxpupuli@groups.io <voxpupuli@groups.io> *On Behalf Of *David
Hollinger via groups.io
*Sent:* Friday, October 29, 2021 9:44 AM
*To:* voxpupuli@groups.io
*Cc:* puppet-users@...
*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.*
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
<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fvoxpupuli%2Fvoxpupuli-puppet-lint-plugins%2Fblob%2Fmaster%2Fvoxpupuli-puppet-lint-plugins.gemspec&data=04%7C01%7CJohn.Bollinger%40StJude.org%7Ccf737d31ff9549b6860a08d99aea80b7%7C22340fa892264871b677d3b3e377af72%7C0%7C0%7C637711154851057833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=I%2FSmlN1tMXANeU7XowSyYW%2F9Uq5u11Ej7yoD58NwMtA%3D&reserved=0>
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
<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprotonmail.com%2F&data=04%7C01%7CJohn.Bollinger%40StJude.org%7Ccf737d31ff9549b6860a08d99aea80b7%7C22340fa892264871b677d3b3e377af72%7C0%7C0%7C637711154851057833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=KWlq3y32NYPtUbV3ki7Xnva7TeIzylTqUMu%2BfCpsXE4%3D&reserved=0>
Secure Email.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, October 28th, 2021 at 3:33 PM, Ben Ford <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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fvoxpupuli%2Fvoxpupuli-puppet-lint-plugins%2Fblob%2Fmaster%2Fvoxpupuli-puppet-lint-plugins.gemspec&data=04%7C01%7CJohn.Bollinger%40StJude.org%7Ccf737d31ff9549b6860a08d99aea80b7%7C22340fa892264871b677d3b3e377af72%7C0%7C0%7C637711154851067794%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Y%2FlqbI0AptAG66MGn1MITMXJMJzISp7aYiTyBoKohXA%3D&reserved=0>
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
<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fvoxpupuli%2Fvoxpupuli-puppet-lint-plugins%2Fblob%2Fmaster%2Fvoxpupuli-puppet-lint-plugins.gemspec&data=04%7C01%7CJohn.Bollinger%40StJude.org%7Ccf737d31ff9549b6860a08d99aea80b7%7C22340fa892264871b677d3b3e377af72%7C0%7C0%7C637711154851067794%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Y%2FlqbI0AptAG66MGn1MITMXJMJzISp7aYiTyBoKohXA%3D&reserved=0>
Thank you all for your help!
--
Ben Ford
@binford2k
Ecosystem Product Manager
------------------------------
Email Disclaimer: www.stjude.org/emaildisclaimer
Consultation Disclaimer: www.stjude.org/consultationdisclaimer
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.
I'm not trying to create a controversial list here, just some basic stuff that might be helpful. These are things that i had to do when getting a module "approved".- Does `pdk validate` pass (metadata lint, etc)?- Does it pass "basic" puppet-lint checks (ones built into PDK for example)?- Are there unit tests and do they pass?- Are there integration tests? Do they pass? For what systems/OS?- Is the module documented with puppet-strings?- Is the README formatted in the "approved" formatting and sections?I think green checkmarks or numeric values (depending on the item) would allow the user to make an informed decision.
This seems like a really good starting point. Here are my thoughts on the list and related bits from other replys:
A key thing on the first one though is what it's doing, not how it's executing. PDK is just wrapping up things that I think might be better validated by something like the meta-gem that defines a list of the puppet-lint checks that Ben mentioned in the original message. IMO, a quality module should utilize all of the following:
- puppet-lint plus several of the available plugins. These could all be pulled in via the aforementioned meta gem.
- metadata.json linting via https://rubygems.org/gems/metadata-json-lint
- syntax checking of puppet manifests and hiera keys via https://rubygems.org/gems/puppet-syntax
- validation of all yaml files via https://rubygems.org/gems/yamllint
- any other checks currently done via pdk validate including ones for tasks and epp
- basic rubocop validation of ruby files
I also like what Ben talked about regarding including security-related checks.
Assuming tests are run and pasing, having these in place prevent many errors from making it into released code and promote writing code in a way that makes it easy for contributors to move from module to module and still understand what's going on.
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.
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.
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.
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
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).
Regarding the gems in the meta lint gem mentioned, I have suggestions I will be putting a PR up for shortly.
---
Gene Liverman
Principal Site Reliability Engineer
On Wed, Nov 3, 2021 at 11:49 AM Martin Alfke <tuxmea@...> wrote:
Hi,
to be honest: I really liked that there was the possibility for user to provide individual feedback on forge.
When it comes to quality, I see the following topics:
- pdk validate passing (without disabling any check globally)
- acceptance tests
- strings documentation
- reference.md as part of the release (also in GIT - many people want to read the reference even when not having strings installed)
- supported puppet versions (older ones are OK, but stable and supported puppet versions should be covered
Additionally I see the following topics covering "active community" question:
- response time on issues (not fixing, but when will the issue author receive the first feedback/information)
- regular releases (minimum bi-yearly or more often)
Best,
Martin
> On 3. Nov 2021, at 14:57, Nick Maludy <nmaludy@...> wrote:
>
> Hey everyone,
>
> I haven't read every single reply here so i apologize if i'm repeating someone else's opinion.
>
> My general thought about trying to "measure quality" as a single number/metric is going to be difficult. As i've seen from several of the responses, "quality" means something different to each individual person or team.
>
> 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'm not trying to create a controversial list here, just some basic stuff that might be helpful. These are things that i had to do when getting a module "approved".
> - Does `pdk validate` pass (metadata lint, etc)?
> - Does it pass "basic" puppet-lint checks (ones built into PDK for example)?
> - Are there unit tests and do they pass?
> - Are there integration tests? Do they pass? For what systems/OS?
> - Is the module documented with puppet-strings?
> - Is the README formatted in the "approved" formatting and sections?
>
> I think green checkmarks or numeric values (depending on the item) would allow the user to make an informed decision.
>
> Thoughts?
>
> -Nick
>
>
> On Wed, Nov 3, 2021 at 9:24 AM Ewoud Kohl van Wijngaarden <ewoud+voxpupuli@...> wrote:
> On Tue, Nov 02, 2021 at 12:10:03PM -0700, Ben Ford wrote:
> >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
> ><https://puppet.com/docs/puppet/7/modules_documentation.html>, and that's
> >honestly a terrible approximation.
>
> That's I tihnk the point most of us were making: real quality is hard to
> determine.
>
> >Right now, what we can realistically evaluate are lint checks
> ><https://github.com/voxpupuli/voxpupuli-puppet-lint-plugins/blob/master/voxpupuli-puppet-lint-plugins.gemspec>
> >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
> ><https://github.com/binford2k/binford2k-itemize> to help surface how a
> >module works without forcing you to read source code.
>
> I would say that you can generally it should be safe to regenerate the
> reference and diff. There are 2 reasons they can then differ. First is
> that the developer forgot this when updating their module. The other is
> that puppet-strings was updated since it was generated (and has changed
> something in their templates).
>
> As for assert_private(), I must admit that I always try to use
> @api private but never assert_private(). As long as the tool can deal
> with that, it sounds like a decent check.
>
> One thing that I can think of is programatically generating spec tests.
> In our module we have a lot of trivial classes that are exposing a bunch
> of plugins. I test them in a loop to avoid a bunch of small files:
> https://github.com/theforeman/puppet-foreman/blob/master/spec/classes/cli_plugins_spec.rb
> Perhaps a bit of an edge case, but I'd hate to lose score points on
> this.
>
> rspec-puppet does have coverage reports:
> https://rspec-puppet.com/documentation/coverage/
> Perhaps that could help? However, it does require running tests which is
> an area I'd personally like to avoid.
>
> >We already have some security lint checks
> ><https://github.com/floek/puppet-lint-security-plugins> on our roadmap. If
> >you attended Puppetize, you might have heard Florian talk about the project.
>
> Is there a recording of the talk?
>
> >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.
>
> I always thought it was standardized via puppetlabs_spec_helper and its
> release_checks task:
> https://github.com/puppetlabs/puppetlabs_spec_helper/blob/a4f45bc4c5c8fabad353550770f98a44acf0d5d7/lib/puppetlabs_spec_helper/rake_tasks.rb#L276-L286
>
> >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 😂
> >
> >
> >
> >On Fri, Oct 29, 2021 at 11:13 AM Bollinger, John C via groups.io
> ><John.Bollinger=StJude.org@groups.io> wrote:
> >
> >> 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:* voxpupuli@groups.io <voxpupuli@groups.io> *On Behalf Of *David
> >> Hollinger via groups.io
> >> *Sent:* Friday, October 29, 2021 9:44 AM
> >> *To:* voxpupuli@groups.io
> >> *Cc:* puppet-users@...
> >> *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.*
> >>
> >>
> >>
> >> 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
> >> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fvoxpupuli%2Fvoxpupuli-puppet-lint-plugins%2Fblob%2Fmaster%2Fvoxpupuli-puppet-lint-plugins.gemspec&data=04%7C01%7CJohn.Bollinger%40StJude.org%7Ccf737d31ff9549b6860a08d99aea80b7%7C22340fa892264871b677d3b3e377af72%7C0%7C0%7C637711154851057833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=I%2FSmlN1tMXANeU7XowSyYW%2F9Uq5u11Ej7yoD58NwMtA%3D&reserved=0>
> >>
> >> 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
> >> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprotonmail.com%2F&data=04%7C01%7CJohn.Bollinger%40StJude.org%7Ccf737d31ff9549b6860a08d99aea80b7%7C22340fa892264871b677d3b3e377af72%7C0%7C0%7C637711154851057833%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=KWlq3y32NYPtUbV3ki7Xnva7TeIzylTqUMu%2BfCpsXE4%3D&reserved=0>
> >> Secure Email.
> >>
> >>
> >>
> >> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> >> On Thursday, October 28th, 2021 at 3:33 PM, Ben Ford <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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fvoxpupuli%2Fvoxpupuli-puppet-lint-plugins%2Fblob%2Fmaster%2Fvoxpupuli-puppet-lint-plugins.gemspec&data=04%7C01%7CJohn.Bollinger%40StJude.org%7Ccf737d31ff9549b6860a08d99aea80b7%7C22340fa892264871b677d3b3e377af72%7C0%7C0%7C637711154851067794%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Y%2FlqbI0AptAG66MGn1MITMXJMJzISp7aYiTyBoKohXA%3D&reserved=0>
> >> 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
> >> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fvoxpupuli%2Fvoxpupuli-puppet-lint-plugins%2Fblob%2Fmaster%2Fvoxpupuli-puppet-lint-plugins.gemspec&data=04%7C01%7CJohn.Bollinger%40StJude.org%7Ccf737d31ff9549b6860a08d99aea80b7%7C22340fa892264871b677d3b3e377af72%7C0%7C0%7C637711154851067794%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Y%2FlqbI0AptAG66MGn1MITMXJMJzISp7aYiTyBoKohXA%3D&reserved=0>
> >>
> >>
> >>
> >> Thank you all for your help!
> >>
> >>
> >>
> >> --
> >>
> >> Ben Ford
> >>
> >> @binford2k
> >>
> >> Ecosystem Product Manager
> >>
> >>
> >>
> >>
> >> ------------------------------
> >>
> >> Email Disclaimer: www.stjude.org/emaildisclaimer
> >> Consultation Disclaimer: www.stjude.org/consultationdisclaimer
> >>
> >>
> >>
> >
> >
> >
> >
> >
>
>
>
>
>
>
Ben Ford
On Thu, Nov 4, 2021 at 8:11 AM Gene Liverman <gene.liverman@...> 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 module2) 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 😜
Nick Maludy
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
On Thu, Nov 4, 2021 at 12:56 PM Ben Ford <ben.ford@...> wrote:
On Thu, Nov 4, 2021 at 8:11 AM Gene Liverman <gene.liverman@...> 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 module2) 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.mdSounds 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 <nmaludy@...> 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.-NickOn Thu, Nov 4, 2021 at 12:56 PM Ben Ford <ben.ford@...> wrote:On Thu, Nov 4, 2021 at 8:11 AM Gene Liverman <gene.liverman@...> 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 module2) 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.mdSounds 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 😜