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

Bump windows cookbook dependency #111

Merged
merged 1 commit into from Sep 25, 2017
Merged

Conversation

spuder
Copy link
Contributor

@spuder spuder commented Sep 21, 2017

Resolves issue #29
Bump windows cookbook version now that windows_package is part of chef 12.
Windows cookbook 3.0.x no longer includes windows_package resource.

I intentionally didn't upgrade to 3.1.x since that version of the windows cookbook requires chef 12.7.x

The windows_zipfile has no breaking changes so there should be no change in behavior.

https://github.com/chef-cookbooks/windows/blob/master/CHANGELOG.md

@codecov-io
Copy link

codecov-io commented Sep 21, 2017

Codecov Report

Merging #111 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #111   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      5           
  Lines         136    136           
=====================================
  Hits          136    136

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d38a98a...32abe47. Read the comment docs.

metadata.rb Outdated
@@ -9,7 +9,7 @@
issues_url 'https://github.com/cvent/octopus-deploy-cookbook/issues'
version '0.12.0'

depends 'windows', '>= 1.38.0'
depends 'windows', '>= 3.0.5'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think we do not need the cookbook anymore. The main reason for having it was simply for backwards compatibility issues.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh forgot about the tools resource which does require it. I would prefer to just not specify a version here TBH and make it so its picked outside of this cookbook.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sever and tentacle don't, but it looks like tools still uses the windows_zipfile resource which I believe does require the windows cookbook.

https://github.com/cvent/octopus-deploy-cookbook/blob/master/resources/tools.rb#L44-L48

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually I believe in pinning cookbook versions, however in this case I think you are right. The likelyhood users will run into dependency hell if they have other cookbooks that depend on the windows cookbook outweighs the risk that the windows_zipfile resource api will break.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if the version >= 1.38.0 was needed or not, One thing you could do is add a dependency on windows >= 3.0.5 in the berksfile so that in testing it would use that version.

@spuder
Copy link
Contributor Author

spuder commented Sep 22, 2017

I've added windows cookbook dependency to berkshelf, and removed the version pin from the metadata file.

@spuder spuder force-pushed the 29-windows_package branch 2 times, most recently from 836f3dd to 0c278b6 Compare September 22, 2017 19:42
Berksfile Outdated
@@ -3,6 +3,8 @@ source 'https://supermarket.chef.io/'

metadata

cookbook 'windows', '~> 3.0.5'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we put this in the test group?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, pushed.

No longer required if using chef >=12.4
@brentm5 brentm5 merged commit 2e8afd8 into cvent:master Sep 25, 2017
@spuder spuder deleted the 29-windows_package branch September 25, 2017 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants