Add a Array[String] argument of repo IDs to be enabled #78

pdemonaco · 2018-09-17T19:07:53Z

Pull Request (PR) description

This request introduces three primary changes

Fact calculation is now constrained to the ['os']['name'] fact instead of the classic 'osfamily' fact. This ensures that the module doesn't waste time running on CentOS or other non RHEL RedHat derivatives.
The custom facts have been combined into a single 'rhsm' custom fact.
A new parameter named enabled_repo_ids has been added to the rhsm class to allow an array of strings.

This Pull Request (PR) fixes the following issues

Directly addresses issue Yumrepo target parameter does not work #68.
Indirectly addresses issue Features request: SCL repos #66 by allowing the desired repo to be specified.

Rubocop wanted the addition of a few trailing commas on the tests as they currently exist. Adding these here ensures that they don't clutter my direct changes.

This commit reworks the main class to accept an array of repo IDs instead of the original boolean values. Each ID is used as the namevar of a repo define entity which uses the `subscription-manager repos --enable` command to activate.

This commit corrects a bug where an undefined repo list would cause the manifest to fail during compilation.

Corrected the os/family filter so that it works. For some reason the Facter.value syntax wasn't actually finding the structured ['os']['name'] fact so I had to convert to a ruby block. Additionally, the aggregation wasn't correct as the return values from each chunk were not hashes and therefore couldn't be merged.

Added the new parameter to both the README and the puppet strings. Also generated a new REFERENCE.md file using the puppetstring function.

pdemonaco · 2018-09-17T19:14:35Z

Huh. Is there a reason that the rubocop configuration for this module expects exactly the opposite behavior for trailing comments than that of the default in pdk validate?

Configure the enabled_repo_ids array to merge unique values when searching in hiera.

This has been updated to meet the build-requirements defined by travis.

bastelfreak · 2018-09-17T21:09:42Z

.rspec

 --color
+--format documentation


why did you change this?

Shoot - I didn't mean to commit that. I converted the module to be PDK compatible for my own local testing during development and I accidentally added this file. PDK changed a ton of stuff and I'm not familiar enough with Travis or the testing configuration in use on this module so I didn't add the other changes. I'll revert this for now.

kallies

Very valuable changes. They require a new major version of the module in my point of view.

kallies · 2018-09-18T05:13:26Z

README.md

@@ -51,6 +51,33 @@ You shouldn't specify the protocol, subscription-manager will use HTTP. For prox

 The proxy settings will be used to register the system and as connection option for all the YUM repositories generated in `/etc/yum.repos.d/redhat.repo`

+### Enabled Repos
+
+A string array of repo IDs can now be provided as an argument to the class definition. This list will be used to enable the target repos if that has not already occurred.


I'd rather say "can be provided" instead of "now".

kallies · 2018-09-18T05:14:02Z

README.md

+class { 'rhsm':
+  rh_user          => 'myuser',
+  rh_password      => 'mypassword',
+  enabled_repo_ids => [


kallies · 2018-09-18T05:14:26Z

README.md

+  rh_user          => 'myuser',
+  rh_password      => 'mypassword',
+  enabled_repo_ids => [
+    'rhel-7-server-rpms',


Has this been tested with non-RHEL repositories, e.g. custom repositories on Satellite 6?

Unfortunately no, I don't have Satellite in my environment so I haven't tested anything like that.

kallies · 2018-09-18T05:17:48Z

lib/facter/rhsm.rb

@@ -0,0 +1,26 @@
+Facter.add(:rhsm, type: :aggregate) do


This fact is a major change. For example, we rely on rhsm_subscription_type for a Satellite 5 to 6 migration.

kallies · 2018-09-18T05:23:27Z

manifests/repo.pp

@@ -12,9 +12,13 @@
 #   ::rhsm::repo { "rhel-${::operatingsystemmajrelease}-server-extras-rpms": }
 define rhsm::repo (


empty braces () might be removed.

Thinking about it again, it would be nice to have a parameter for disabling repositories, too. Or better than nice: we need this functionality but solve it in a additional module. It will simplify the configuration.

lib/facter/rhsm_repos.rb

This change includes some minor tweaks to the readme file as well as adding in a build status and puppet version badge.

Added in the original rhsm_repos and rhsm_subscription_type facts for backward compatibility. These facts are now derived from the new rhsm structured fact to avoid doubling work.

pdemonaco · 2018-09-18T19:08:01Z

@kallies should I add a version bump in this pull request? I'm assuming by major you mean 4.x.x?

kallies · 2018-09-20T11:45:14Z

@kallies should I add a version bump in this pull request? I'm assuming by major you mean 4.x.x?

Yes, 4.0.0 would be nice. If there are no braking changes (no fact removal etc.) 3.2.0 is fine for me, too.

@bastelfreak should the version bump be a separate PR?

lib/facter/rhsm.rb

Added an explicit check to the enabled repos facts such that the fact will only be calculated if the subscription-manager command exists. This allows the fact to run on CentOS machines which have subscription-manager installed and prevents errors on those that do not.

An incorrect indendation was left within the enabled_repo_ids chunk when the test for subscription-manager was moved outside of the chunk. Should be all set now.

manifests/init.pp

spec/default_facts.yml

spec/defines/repo_spec.rb

bastelfreak · 2018-09-23T08:33:50Z

Because of the changed fact this is a breaking change and requires a new major release. You do not need to bump any version here, this will be triggered through the label I applied, during the next release. However, we need to ensure that an upgrade works smoothly for the users. I'm currently not sure how we should properly test it.

pdemonaco · 2018-09-23T18:04:28Z

@bastelfreak is this still backwards incompatible with the re-introduced 'rhsm_' facts? I also converted the confines statement to target the RedHat family again but caused it to skip execution for the list of enabled repos if the subscription-manager binary is missing.

The following corrections are included in this commit: * enabled_repo_ids is nolonger optional, however, an empty array can be provided * Legacy syntax left in previous releases has been removed * PDK conversions which were accidentally committed have been removed * Compile with all deps is now selected.

bastelfreak · 2018-09-23T19:13:06Z

README.md

+
+## Development
+
+Some general guidelines on PR structure can be found [here](https://voxpupuli.org/docs/#reviewing-a-module-pr).


neat, we should add that to all modules.

manifests/init.pp

bastelfreak · 2018-09-23T19:59:35Z

@pdemonaco you're right, I think this is fine now. Please align the = and afterwards we can merge this.

Alignment of the equal signs on the interface to the primary class.

pdemonaco · 2018-09-24T14:42:30Z

@bastelfreak will this generate a new version on the puppet forge automatically?

bastelfreak · 2018-09-25T06:57:08Z

No, we will have to do a new release, maybe in the next few days.

ubellavance · 2018-10-04T14:18:44Z

That looks like an awesome PR! I can't wait to try it (when I find time to do so). Thanks!

pdemonaco added 5 commits September 14, 2018 13:23

Existing Test Ruby Tweaks

f001830

Rubocop wanted the addition of a few trailing commas on the tests as they currently exist. Adding these here ensures that they don't clutter my direct changes.

Subscription Manager - Repo Enable

117d5a8

This commit reworks the main class to accept an array of repo IDs instead of the original boolean values. Each ID is used as the namevar of a repo define entity which uses the `subscription-manager repos --enable` command to activate.

Handle Undefined Repo List

c56da58

This commit corrects a bug where an undefined repo list would cause the manifest to fail during compilation.

Documentation Update

b544d1d

Added the new parameter to both the README and the puppet strings. Also generated a new REFERENCE.md file using the puppetstring function.

pdemonaco added 2 commits September 17, 2018 15:25

Repo List Merge Strategy

5af2008

Configure the enabled_repo_ids array to merge unique values when searching in hiera.

Trailing Comma Argument Correction

523b653

This has been updated to meet the build-requirements defined by travis.

pdemonaco changed the title ~~Feature/master/repo list argument~~ Add a Array[String] argument of repo IDs to be enabled Sep 17, 2018

bastelfreak reviewed Sep 17, 2018

View reviewed changes

kallies reviewed Sep 18, 2018

View reviewed changes

lib/facter/rhsm_repos.rb Show resolved Hide resolved

pdemonaco added 2 commits September 18, 2018 09:33

Documentation & Minor PDK Related Unroll

25bdbb7

This change includes some minor tweaks to the readme file as well as adding in a build status and puppet version badge.

Fact Backward Compatibility

0d804c9

Added in the original rhsm_repos and rhsm_subscription_type facts for backward compatibility. These facts are now derived from the new rhsm structured fact to avoid doubling work.

kallies reviewed Sep 20, 2018

View reviewed changes

lib/facter/rhsm.rb Outdated Show resolved Hide resolved

pdemonaco added 2 commits September 21, 2018 10:08

rhsm Fact Spacing Issue

ae3ee69

An incorrect indendation was left within the enabled_repo_ids chunk when the test for subscription-manager was moved outside of the chunk. Should be all set now.