Skip to content
This repository has been archived by the owner on Feb 21, 2018. It is now read-only.

Adds basic spec testing, fixes empty string and other Puppet 4 requirements #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

petems
Copy link

@petems petems commented Aug 22, 2016

  • This is the minimum requirement to get to Puppet 4 support in the module, as there’s also a string comparison issue with integers in yum.pp
  • The empty string is true in the Puppet 4+ parser. undef should be used instead for a false value for strings.
  • Basic boilerplate for unit testing of classes

* This is the minimum requirement to get to Puppet 4 support in the module, as there’s also a string comparison issue with integers in `yum.pp`
* The empty string is true in the Puppet 4+ parser. `undef` should be used instead for a false value for strings.
* Basic boilerplate for unit testing of classes
@petems
Copy link
Author

petems commented Aug 22, 2016

@mschwager Mind merging this when you get the chance? I've started to use Duo recently and have a bunch of refactoring and fixes for this module, having basic testing in would help a lot! 👍

Also, would it be possible to enable Travis on this repo with the .travis.yml file I've added? Should be able to help with a lot of the Puppet 4 changes required

@mlhess
Copy link
Contributor

mlhess commented Sep 13, 2016

+1 to merging this

@mschwager
Copy link
Contributor

Hey guys! Sorry for taking so long to respond, this project is unfortunately low priority for us.

Is there any way we could distill this PR down to the minimum necessary to support Puppet 4? For example, would we be OK to just take the changes from init.pp, yum.pp, and spec/classes/duo_unix_spec.rb?

Also, what does adding support for Ruby packaging afford us (my Puppet knowledge is lacking...)?

@petems
Copy link
Author

petems commented Sep 13, 2016

@mschwager The Gemfile is dependency management for rspec tests, basically unit testing for Puppet. I can remove the Gemfile.lock if needed. Is there a reason you'd want them removed?

If this module is lower priority, would you be open to adding maintainers? as someone who uses Duo and Puppet together, I would love to help out! 👍

@mschwager
Copy link
Contributor

Great! Yeah, adding maintainers, or even transferring ownership is certainly a possibly. I'll look into in :)

As for this PR, I'll hold off until a verdict is reached for the above decision since that would help move this along.

@petems
Copy link
Author

petems commented Sep 13, 2016

If that is a blocker and might take some time, id much prefer a merge of a slimmed down version of this PR just for Puppet 4 support, so people downloadong the module from the forge can get a working version. Id really like it to be simple for Puppet users using the latest Puppet version to use duo unix! 😆

So I can split the testing from this PR and that can wait on that decision, then get this PR merged?

@mschwager
Copy link
Contributor

@petems I'll get an answer for you by the middle of next week.

@petems
Copy link
Author

petems commented Sep 15, 2016

👍 Thanks!

@mschwager
Copy link
Contributor

@petems, sorry for the delay.

Our thoughts on transferring ownership:

We greatly appreciate you contributing your time and effort to help us with this project. This transfer is definitely something we'd like to undertake, but we're having a difficult time coming up with a good way to do it. There are security concerns around transferring ownership of the project, which effectively gives arbitrary code execution, to someone not affiliated with Duo. Our users and customers would be hard-pressed to know that we've transferred ownership of a product out from under them.

Potential solutions:

  • You could hard fork the project. This would then divide the community, which isn't ideal.
  • We could continue to work together with pull requests and issues. This may be the best option given the aforementioned concerns.
  • Transfer ownership. This would require a long-term plan for accommodating this change.

What are your thoughts?

@petems
Copy link
Author

petems commented Oct 4, 2016

@mschwager No worries, I imagine there's a fair few stakeholders that this needs to be discussed with.

In terms of options, I'd prefer the second. It's probably best to keep the module under the Duosecuriy Github namespace, as you say, customers and users are going to be more comfortable using a module to install when it's under the official namespace.

The changes in this PR are fairly low touch: they are backwards compatible with older Puppet, but also allow using the new Puppet 4 parser, plus add spec testing to allow unit testing of future changes. Puppet < 4 is slowly becoming EOL'd, especially in an Enterprise context.

Maybe if there needs to be a massive overhall or big refactoring in the future, the moving of this module could be discussed to something like Vox Puppuli (a group that takes orphaned modules and maintains them for the community). But that's probably something to leave for the future for now.

tl;dr Lets stick with staying in the Duo namespace for now, get the PR merged and maybe discuss deprecating and transferring to the community if there needs to be a big overhall in the future.

@davealden
Copy link

@mschwager I would recommend removing this module if you don't have the time to merge in patches that allow this to work with puppet4. By leaving this up, all you've succeeded in doing is wasting folks time (especially those who grab the module thinking it works in puppet4 since you do not have it listed as not working with puppet4).

@adrikim adrikim mentioned this pull request Jul 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants