Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add datatypes & multiple enhancements #107

Merged

Conversation

@genebean
Copy link

genebean commented Nov 15, 2019

We have had a fork for a couple of years now and I am hoping we can contribute our changes to the Vox Pupuli version of this module so that we no longer need it. This is a combination of the changes below. Attribution for the original work is done via the co-author lines at the end of the commit message

  • Allow passing options to createrepo in mrepo.conf
  • Add createrepo_options variable to package class
  • Add regex validation for ppc64le architecture
    • Modified manifests/repo.pp and added a regex statement for validating ppc64le architecture when used in the $arch param. (I modified the original commit to update the new type used upstream while resolving merge conflicts)
  • Additional changes based on work upstream
    • Added a spec test to cover createrepo_options
    • Fixed duplicate resource declaration in iso.pp
    • Fixed passing params through to ncc and rhn repo types
    • Typed all parameters and replaced empty strings with undef
    • Removed .empty? checks from erb due to Puppet type checking doing this
    • Replaced legacy and top-scope facts
@genebean genebean changed the title Upstreaming local mods (WIP) Upstreaming local mods Nov 15, 2019
@genebean genebean force-pushed the puppetlabs-operations:move_to_vox_version branch 10 times, most recently from c388226 to a12baf6 Nov 15, 2019
manifests/init.pp Outdated Show resolved Hide resolved
manifests/params.pp Outdated Show resolved Hide resolved
@genebean genebean force-pushed the puppetlabs-operations:move_to_vox_version branch 7 times, most recently from 7200322 to 94c0876 Nov 18, 2019
@genebean genebean changed the title (WIP) Upstreaming local mods Upstreaming local mods Nov 18, 2019
@genebean genebean force-pushed the puppetlabs-operations:move_to_vox_version branch 3 times, most recently from b02fb55 to f1cb1e1 Nov 18, 2019
This is a combination of the changes below. Attribution for the original
work is done via the co-author lines at the end of this commit.

- Allow passing options to createrepo in mrepo.conf
- Add createrepo_options variable to package class
- Add regex validation for ppc64le architecture
  Modified manifests/repo.pp and added a regex statement for validating
  ppc64le architecture when used in the $arch param.
- Additional changes based on work upstream
  - Added a spec test to cover createrepo_options
  - Fixed duplicate resource declaration in iso.pp
  - Fixed passing params through to ncc and rhn repo types
  - Typed all parameters and replaced empty strings with undef
  - Removed .empty? checks from erb due to Puppet type checking doing this
  - Replaced legacy and top-scope facts

Co-authored-by: Rob Braden <bradejr@puppetlabs.com>
Co-authored-by: Eric Zounes <zounes99@gmail.com>
Co-authored-by: Heston Snodgrass <hsnodgrass3@gmail.com>
Co-authored-by: Erik Hansen <suckatrash@users.noreply.github.com>
@genebean genebean force-pushed the puppetlabs-operations:move_to_vox_version branch from f1cb1e1 to 4d29ce8 Nov 18, 2019
@genebean

This comment has been minimized.

Copy link
Author

genebean commented Nov 18, 2019

@bastelfreak I think I have all issues resolved and tests are passing. I have updated the initial PR comment and the commit message to reflect current reality. While working on the type'ing bit you mentioned I realized that all the parameters needed to be typed so I did it :)

@bastelfreak bastelfreak changed the title Upstreaming local mods Add datatypes & multiple enhancements Nov 18, 2019
@bastelfreak bastelfreak removed the needs-work label Nov 18, 2019
@bastelfreak

This comment has been minimized.

Copy link
Member

bastelfreak commented Nov 18, 2019

thanks!

@bastelfreak bastelfreak merged commit 96dfb2d into voxpupuli:master Nov 18, 2019
2 checks passed
2 checks passed
WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@genebean genebean deleted the puppetlabs-operations:move_to_vox_version branch Nov 18, 2019
@genebean

This comment has been minimized.

Copy link
Author

genebean commented Nov 18, 2019

Pure curiosity @bastelfreak - do you know when you will cut a new version for the Forge?

@bastelfreak

This comment has been minimized.

Copy link
Member

bastelfreak commented Nov 18, 2019

I need to update the vcsrepo dependency in #108, afterwards I can do a new release.

@mrolli

This comment has been minimized.

Copy link
Contributor

mrolli commented on manifests/repo.pp in 4d29ce8 Dec 11, 2019

This is plain wrong. According the documentation of mrepo $metadata has to be of type Enum['yum', 'repomd', 'repoview']. This change breaks code!

This comment has been minimized.

Copy link

genebean replied Dec 11, 2019

@mrolli I’m totally happy to push a change so that it’s an Enum but don’t quite understand how this is “plain wrong.” The default is on of the valid options you enumerated and the parameter allows for any string so you could easily set it to ‘yum’ or ‘repoview.’ Can you explain the issue you see more so that we can create the most accurate fix?

This comment has been minimized.

Copy link
Contributor

mrolli replied Dec 11, 2019

One should be able to take more than one option! This is from an mrepo perspective a valid metadata value:
$metadata => ['yum', 'repomd', 'repoview']

This is perfectly valid and the values that we run on. Your addition of String[1] broke this. No offense in my lines! I'll prepare a PR to fix this.

This comment has been minimized.

Copy link
Contributor

mrolli replied Dec 11, 2019

This is the datatype I'm preparing right now:

 Variant[String[1],Array[Enum['yum','apt','repomd','repoview']]] $metadata = 'repomd

to be backward compatible

This comment has been minimized.

Copy link

genebean replied Dec 11, 2019

Ahhh, that makes perfect sense. Thanks for helping me understand!

This comment has been minimized.

Copy link
Contributor

mrolli replied Dec 11, 2019

You're welcome. PR is on.

This comment has been minimized.

Copy link
Contributor

mrolli replied Dec 11, 2019

Actually, String[1] should also be the Enum again. Silly me.

@@ -156,7 +157,7 @@
# If undefined and selinux is present and not disabled, use selinux.
case $mrepo::selinux {
undef: {
case $::selinux {
case $selinux {

This comment has been minimized.

Copy link
@alexjfisher

alexjfisher Dec 12, 2019

Member

This looks incorrect and like it was also always broken.
Problem 1) You’ve switched from the selinux fact to the local variable of the same name.
Problem 2) The selinux core fact is actually a Boolean so would have never resolved to enforcing or permissive.

This comment has been minimized.

Copy link
@alexjfisher

alexjfisher Dec 12, 2019

Member

Turns out I’ve noticed this before! #56 (comment) :)

This comment has been minimized.

Copy link
@alexjfisher

alexjfisher Dec 12, 2019

Member

I've opened #112 that should fix it .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.