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

Loosen the pinning on "ohai" #823

Merged
merged 1 commit into from
Feb 17, 2018
Merged

Conversation

richardc
Copy link
Contributor

@richardc richardc commented Feb 13, 2018

Description

Ohai requirement of ~> 8.0 is incompatible with Chef 13 and newer

Unpin the requirement a little to allow for Ohai 14.x, as we expect the Ohai api used by Omnibus is stable through to at least Chef/Ohai 14.

Closes #801


Maintainers

Please ensure that you check for:

  • [] If this change impacts git cache validity, it bumps the git cache
    serial number
  • [] If this change impacts compatibility with omnibus-software, the
    corresponding change is reviewed and there is a release plan
  • [] If this change impacts compatibility with the omnibus cookbook, the
    corresponding change is reviewed and there is a release plan

@richardc richardc requested a review from a team as a code owner February 13, 2018 17:30
@richardc richardc changed the title Loosen the pinning on "ohai" WIP: Loosen the pinning on "ohai" Feb 13, 2018
@lamont-granquist
Copy link
Contributor

this should likely just be < 15

i doubt omnibus cares which version of ohai it actually uses and this is mostly used to prevent against (very unlikely) future major version breaking changes in ohai.

and chef/ohai 14 is coming soon and that should be the ultimate target.

@lamont-granquist
Copy link
Contributor

(and i checked the git blame history back awhile and i can't see any rationale for the pinning here other than simply to pin 'cuz pinning).

@scotthain
Copy link
Contributor

I'm fine with < 15

@richardc
Copy link
Contributor Author

Thanks for the review, I've bumped it to use < 15

@richardc richardc changed the title WIP: Loosen the pinning on "ohai" Loosen the pinning on "ohai" Feb 14, 2018
@richardc
Copy link
Contributor Author

richardc commented Feb 14, 2018

I see the appveyor builds for RUBY 2.2 are failing now. The previous run for this PR solved to ohai 8.26.1 but subsequent runs are solving to 13.7.1 which fails to installl as v13.0.0 pushed the requirement up to ruby 2.3 https://github.com/chef/ohai/blob/master/CHANGELOG.md#v1300-2017-04-06

What's the best direction to take here? Vary the dependency based on ruby version, or drop ruby 2.2 testing from the test matrix?

@richardc
Copy link
Contributor Author

I was confused as to this resolution though - as the gem correctly advertises it's required ruby version - Looking closer though this is a feature that's only available in bundler v1.13 and the version used is 1.10.6. Will try bumping to the last in the v1.13 series (1.13.7) to see if that improves matters.

@richardc
Copy link
Contributor Author

Actually that last change has no effect, and I should have seen, because though the appveyor job installs a version of bundler, the version that gets used is 1.16.1. 🤔 I guess that was needed in the past to upgrade something forwards.

@richardc
Copy link
Contributor Author

Varying the dependency in the Gemfile so it's just scoped to invoking bundler for the CI runs is the approach taken in ebd5ba8. It's probably not the cleanest way to approach this, but it does fix the build errors on appveyor's ruby 2.2 cell.

@lamont-granquist
Copy link
Contributor

I'd be 👍 on bumping the required_ruby_version here up to 2.3 and nuking 2.2 out of the matrix.

@richardc
Copy link
Contributor Author

Sold. 7e07e68 loosens the pinning, bumps the minimum ruby version, and drops the 2.2 test from the appveyor matrix.

@richardc
Copy link
Contributor Author

55a40b7 drops it from travis too. ☕️

@scotthain
Copy link
Contributor

@richardc one last thing - could you also update the README to say 2.3+ ? I think that'll be the final piece.

Thanks for all your work on this!

@lamont-granquist
Copy link
Contributor

👍 LGTM

but yeah, should update the README.md

Ohai requirement of ~> 8.0 is incompatible with Chef 13 or newer.

Unpin the requirement a little to allow for Ohai up to 14.x, as for
omnibus's needs the Ohai API should be stable up through Chef/Omnibus 14

As we may now resolve to a dependency that requires ruby >= 2.3, also
bump Omnibus's minimum required ruby >= 2.3 in the gemspec, updates the
README to specify ruby 2.2, and drops the 2.2 testing from appveyor and
travis.

Signed-off-by: Richard Clamp <richardc@unixbeard.net>
@richardc
Copy link
Contributor Author

README.md updated in 58b0034.

expeditor/config-validation is failing - it seems like a new check from before, and I don't really know where to look to address it

@thommay thommay merged commit c75cd8e into chef:master Feb 17, 2018
@thommay
Copy link
Contributor

thommay commented Feb 17, 2018

Thanks for getting through all this @richardc, this is a handy fix!

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.

5 participants