Standard module options #1131

reidmv · 2021-03-04T18:34:32Z

This PR is primarily designed to sanitize the schema used by the Hash source type to define environments and modules, adopting a standard way of describing content (environments, modules) which is not implementation-dependent, the way today's modules especially are.

The idea is that when specifying content in a data hash, it should be possible to specify it using these standard keys.

---
object-name:
  type: <string> # E.g. "git", "forge", "svn"
  source: <string> # E.g. "git://server.tld/project/repo.git", "https://forge.puppet.com", etc.
  version: <string> # E.g. "main", "06a32bc", "6.1.0", etc.

This PR assumes #1130 exists to improve the visibility of what happens if users use conflicting ways of specifying values like version, such as by using both version and an implementation-specific key like git, but does not depend on that improvement.

This PR homogenizes how options are parsed for many objects by standardizing on the use of R10K::Util::Setopts.

See the rich diff of the doc/dynamic-environments/configuration.mkd page for the best visual of what this change accomplishes for consumers.

This improvement takes a UX step in the direction of r10k being able to support other types of environment sources and module sources, such as e.g. tarballs from Artifactory or S3.

This PR should be backwards compatible, without behavior changes, with one exception. Some object types did not validate schema in any way for keys passed into them. R10K::Util::Setopts does validate schema, and so garbage keys that would have been ignored previously may now cause an error.

lib/r10k/module/svn.rb

justinstoller · 2021-03-09T21:54:00Z

Overall, I really like it.

However, I'm also really concerned about there being garbage in folks repository definitions. I haven't looked far enough into it to know if that concern is valid though.

reidmv · 2021-03-09T22:44:45Z

@binford2k can you take a look at this and @justinstoller's comment, let me know what you think?

For now I'll whip up a commit permitting and warning on garbage parameters.

binford2k · 2021-03-09T23:17:56Z

@reidmv is there a pressing reason to disallow garbage parameters? Warn/permit seems like a reasonable non-breaking solution, with the expectation that next major release would make it invalid.

reidmv · 2021-03-11T02:20:59Z

@binford2k @justinstoller I've added some commits that permit R10K::Util::Setopts to warn on unhandled options instead of raising, and R10K::Environment::Git now makes use of that. It looks to me like that was the only object, actually, that wasn't already validating input options somehow. In any case though, it's now possible to permit unhandled options when necessary to preserve behavior.

justinstoller · 2021-03-11T21:03:29Z

@mwaggett had mentioned in the pre-req PR that we should be updating the changelog as we go - we should do that here. Did you want to also review this PR @mwaggett ?

mwaggett · 2021-03-11T21:17:39Z

yes, this should also include a changelog update (and add one for the other PR while you're at it, please 😊 ).

I was planning to review this eventually, but don't wait on me if you think it's already received sufficient review.

reidmv · 2021-03-11T21:29:16Z

New commit pushed with additional docs updates, changelog additions

doc/puppetfile.mkd

lib/r10k/environment/base.rb

lib/r10k/environment/git.rb

lib/r10k/module/forge.rb

lib/r10k/module/svn.rb

spec/unit/module_spec.rb

spec/unit/module/forge_spec.rb

doc/puppetfile.mkd

lib/r10k/environment/svn.rb

spec/unit/module/forge_spec.rb

mwaggett · 2021-03-13T00:03:32Z

Anything you wanna squash @reidmv ? I take it this probably shouldn't all be a single commit, but there are pieces that could be squashed.

reidmv · 2021-03-13T00:04:05Z

Yeah, lemme clean it up a bit. Just a moment.

Let setopts explicity ignore some arguments, and give it the ability to warn on options that don't match the schema, instead of raising. This is to enable using setopts in places where we need to preserve backwards compatibility with a less clearly defined, more permissive interface.

reidmv · 2021-03-13T00:45:52Z

Rebased into something resembling a clean history. There is one final cleanup change mixed in here as well. It relates to how :puppetfile_name is handled. Some tests added to help verify that passing options managed in base classes (such as :puppetfile_name) all works correctly.

mwaggett

Not sure if you wanted to squash in the various pieces of that last improvements commit, but otherwise looks good!

@justinstoller did you want to take another look?

Allow modules of any currently supported type to be initialized using the following standard specification: --- <module-name>: type: <type> source: <source> version: <version> This is in slight contrast to today's expectations, which are largely based on the presence of a type-specific key to indicate the kind of module to create, and there are no standard parameters to indicate source and version. By providing a standard schema for defining modules of any type, this paves the way for easier integration with external data systems which may define sets of modules of many types to include in different environments. This schema is generalized away from implementation and towards the concepts that matter to the user.

@Args

And make it obvious which options are part of the standard interface. Clean up :puppetfile_name handling The pattern appears to be, call super first. The base class initialize method saves off the original, unmodified options to @Args. Base classes then delete options they process. The outer class uses setopts to process the remainder.

Add standard interface updates to docs, changelog Co-authored-by: Molly Waggett <mwaggett@alumni.reed.edu>

reidmv · 2021-03-14T21:13:59Z

Alright, totally squashed, ended with four total commits: 1 for setopts enhancements, 1 for module interface additions, 1 for environment interface additions, and a separate commit for docs updates.

@metadata

The PR puppetlabs#1131 changed the order of method calls in the Forge initialization; that ended up breaking scenarios where the opts was not a Hash and the else branch in the logic called `current_version`, which needs the @metadata object defined before it can be successfully called. This change just simply restores the previous ordering.

@metadata

The PR puppetlabs#1131 changed the order of method calls in the Forge initialization; that ended up breaking scenarios where the opts was not a Hash and the else branch in the logic called `current_version`, which needs the @metadata object defined before it can be successfully called. This change moves that instantiation up to happen before that `current_version` could be called.

@metadata

The PR puppetlabs#1131 changed the order of method calls in the Forge initialization; that ended up breaking scenarios where the opts was not a Hash and the else branch in the logic called `current_version`, which needs the @metadata object defined before it can be successfully called. This change moves that instantiation up to happen before that `current_version` could be called.

@metadata

The PR puppetlabs#1131 changed the order of method calls in the Forge initialization; that ended up breaking scenarios where the opts was not a Hash and the else branch in the logic called `current_version`, which needs the @metadata object defined before it can be successfully called. This change moves that instantiation up to happen before that `current_version` could be called.

@metadata

The PR puppetlabs#1131 changed the order of method calls in the Forge initialization; that ended up breaking scenarios where the opts was not a Hash and the else branch in the logic called `current_version`, which needs the @metadata object defined before it can be successfully called. This change moves that instantiation up to happen before that `current_version` could be called.

reidmv requested a review from a team March 4, 2021 18:34

reidmv force-pushed the standard-module-options branch 2 times, most recently from 779a1a8 to 7555154 Compare March 4, 2021 19:06

justinstoller reviewed Mar 9, 2021

View reviewed changes

lib/r10k/module/svn.rb Outdated Show resolved Hide resolved

reidmv force-pushed the standard-module-options branch from 7555154 to 9bbcd5f Compare March 9, 2021 22:43

reidmv force-pushed the standard-module-options branch 2 times, most recently from 9e90783 to 606cbd5 Compare March 11, 2021 21:27

mwaggett self-requested a review March 12, 2021 22:10

mwaggett reviewed Mar 12, 2021

View reviewed changes

doc/puppetfile.mkd Outdated Show resolved Hide resolved

lib/r10k/environment/svn.rb Outdated Show resolved Hide resolved

spec/unit/module/forge_spec.rb Show resolved Hide resolved

reidmv force-pushed the standard-module-options branch from 029560c to d873f13 Compare March 13, 2021 00:43

mwaggett approved these changes Mar 13, 2021

View reviewed changes

reidmv and others added 3 commits March 14, 2021 14:09

Document new general options for hash sources

a70e301

Add standard interface updates to docs, changelog Co-authored-by: Molly Waggett <mwaggett@alumni.reed.edu>

reidmv force-pushed the standard-module-options branch from d873f13 to a70e301 Compare March 14, 2021 21:10

mwaggett merged commit 93d38cb into puppetlabs:master Mar 15, 2021

reidmv deleted the standard-module-options branch March 17, 2021 19:24

tvpartytonight mentioned this pull request Mar 23, 2021

(maint) Change Forge initialization order #1138

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standard module options #1131

Standard module options #1131

reidmv commented Mar 4, 2021 •

edited

Loading

justinstoller commented Mar 9, 2021

reidmv commented Mar 9, 2021

binford2k commented Mar 9, 2021

reidmv commented Mar 11, 2021

justinstoller commented Mar 11, 2021

mwaggett commented Mar 11, 2021 •

edited

Loading

reidmv commented Mar 11, 2021

mwaggett commented Mar 13, 2021

reidmv commented Mar 13, 2021

reidmv commented Mar 13, 2021 •

edited

Loading

mwaggett left a comment

reidmv commented Mar 14, 2021 •

edited

Loading

Standard module options #1131

Standard module options #1131

Conversation

reidmv commented Mar 4, 2021 • edited Loading

justinstoller commented Mar 9, 2021

reidmv commented Mar 9, 2021

binford2k commented Mar 9, 2021

reidmv commented Mar 11, 2021

justinstoller commented Mar 11, 2021

mwaggett commented Mar 11, 2021 • edited Loading

reidmv commented Mar 11, 2021

mwaggett commented Mar 13, 2021

reidmv commented Mar 13, 2021

reidmv commented Mar 13, 2021 • edited Loading

mwaggett left a comment

Choose a reason for hiding this comment

reidmv commented Mar 14, 2021 • edited Loading

reidmv commented Mar 4, 2021 •

edited

Loading

mwaggett commented Mar 11, 2021 •

edited

Loading

reidmv commented Mar 13, 2021 •

edited

Loading

reidmv commented Mar 14, 2021 •

edited

Loading