-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Major update #9
Major update #9
Conversation
I really like some of these changes so thank you.
|
Hi Liam, Excellent questions! I figured I'd start with the PR as far forward as I already took it for my project use, and if necessary whittle things down based on the code review. With that background, here is my rationale for using remote_file and Puppet 4:
The switch to Puppet 4 is of course what would necessitate making this a new major version. |
From a code mgmt perspective I suggest this. Ideally it could/would apply to every module in p-c. Cutover:
Ongoing:
|
Ok, I am happy for this change to go ahead and I will do all the required stuff to release a new major version on this. @reidmv can you fix the tests first on this? Thanks. |
I can, but not soon. I'm pretty swamped until the end of October. If I get a chance to slip in more work on this before then I will. I'll post an update by October 27. |
I'm trying to get the tests passing in #10 |
@liamjbennett @nibalizer tests cleaned up and PR rebased. Ready for review. |
/^Windows (XP|Vista|7|8|8.1).*/: { $type = 'package' } | ||
default: { $type = 'err' err("dotnet ${version} is not support on this version of windows") } | ||
case $windows_version { | ||
/^2008/, /^2012/: { $type = 'feature' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Windows server 2008 (non R2), dotnet 3.5 is a package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vincent-aguillaume - I've added a commit to fix the error.
@liamjbennett @nibalizer bump |
Hello: What's the status of this PR? Can it be rebased and squashed? |
Rebased and squashed. Note: tests are breaking due to bug, rodjek/rspec-puppet#361. Not sure what to do about that. Works with rspec-puppet 2.3.0 |
@reidmv can you update to rspec-puppet 2.3.2 and try again? |
2ddda63
to
ae4c4a4
Compare
@liamjbennett seems to work! Needed to do style cleanup for rubocop to get that to pass, but the Travis build is green now. |
Can this be squashed? |
@jyaworski squashed. |
Now modified (and squashed) to comply with strict variables. |
@@ -16,7 +16,7 @@ source 'https://rubygems.org' | |||
|
|||
group :test do | |||
gem 'rake' | |||
gem 'puppet', ENV['PUPPET_VERSION'] || '~> 3.8.0' | |||
gem 'puppet', ENV['PUPPET_VERSION'] || '~> 4.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jyaworski This PR is proposing a major version update, and uses Puppet 4 language features. Therefore it is not compatible with any version of Puppet <4.0. I wrote a little more about that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Gemfile is (or will be) managed by modulesync.
https://github.com/voxpupuli/modulesync_config
Please raise a PR over there about it, or this will get overwritten the next time we sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added an issue. voxpupuli/modulesync_config#102.
c96d57a
to
ab9392f
Compare
@jyaworski PR refactored to take modulesync into account. |
Hi @reidmv @liamjbennett, how is it planned to process here? @liamjbennett do you want to do the release (we should do another modulesync before that)? |
Yes I want this to be merged into master. I am not in a position to do the release myself at the moment but if someone else can prepare it on my behalf it would be greatly appreciated. |
@liamjbennett can we talk about the further steps on IRC? I'm currently doing a modulesync in #20, I would like to release the module in the next week. |
ec63307
to
80ab699
Compare
Previously a Powershell exec was used to install packages. This commit converts that task to a package-based resource. It allows cleaner, easier to read code. Switch from download_file to remote_file. This allows for better subscribe/notify of the dotnet defined type as remote_file does not have any intermediate file fragment resources which may be used or changed even if the file to download is not changed. The remote_file resource is also platform agnostic, unlike download_file, more meaning it may be accessible and familar to a wider range of potential users/contributors. Update for Puppet 4 and remove deps Use Puppet 4's type system to deal with validation and remove dependency on stdlib. Use the $::os fact to get Windows release version and remove dependency on windowsfacts. Add logic to prevent incompatible versions Some versions of .NET are in-place upgrades of others. Installation of .NET 4.5, for example, will replace the 4.0 package. In order to disallow Puppet from continuously trying to install .NET 4.0 in the event dotnet resources for both 4.0 and 4.5 have been added to the catalog, create a package=absent resource for each conflicting version. This will cause the conflict to be caught when a catalog is compiled for the node. Add ability to recognize built-in .NET versions Some OSes have some versions of .NET built in and so do not need to have it installed via package. It seems likely this was auto-detected based on registry key presence/absence via exec previously. Because we're now managing the package directly, we should be more explicit about whether or not the package actually needs to be installed. Clean up installation type detection Regex adjustments to make it easier to read which versions support which installation types. Only test on 4.x/future parser Set gemfile to use PUPPET_GEM_VERSION and set default to 4.0 Adjust tests to match updates This is not a comprehensive test overhaul, but does update the functional tests to work with the updated code. Logic correction - For 2008, .NET 3.5 is a package Previously the dotnet define logic would attempt to install .NET 3.5 as a feature on Server 2008. This was incorrect, as in Server 2008 .NET 3.5 was not available as a feature and needed the package installation. Add Dism install type for Windows 7 Windows 7 does not install .NET as a package, nor does it have the ServerManager module. Therefore provide an alternative means of managing the feature via DISM. Fix non-functional onlyif in powershell exec The Puppet Exec resource uses the return code of command, unless, and onlyif to determine success/failure or if action is necessary. Previously, the onlyif in dotnet::install::feature would never exit with a non-zero exit code, even if the Test-Path command returned a False object. Return object and exit code are not the same thing. This commit modifies the onlyif to ensure that if the Test-Path command returns false a non-zero exit code will occur. (style) Line up case blocks Add feature parameter to dotnet::install::feature This will allow specification of different feature names. Necessary specifically because the feature name to install .NET 3.5 on Server 2012 is not the same feature as to install 4.5. The feature name for .NET 3.5 in Server 2012 is not AS-NET-Framework, which we use as the default feature. This commit also updates the dotnet type to attempt installation of the correct feature when 3.5 is specified on Server 2012.
closing because of no progress since over a year |
This PR is a proposed refactor and major version update. This would be released as a new major version.
$::os
fact