search

thoughtbot / terrapin

Run shell commands safely, even with user-supplied values
Other
244 stars 20 forks source link

Loosen dependency lock on ClimateControl (allow 1.x) #15

Closed geoffharcourt closed 8 months ago

geoffharcourt commented 3 years ago

ClimateControl's 1.0 release doesn't have any breaking API changes (the major version bump is a reflection of dropping support for some older Rubies). This change loosens the dependency lock for climate_control to allow any version in the 1.x series to be used. This allows Terrapin and kt-Paperclip to coexist with the new ClimateControl release.

ClimateControl 1.0 release notes: https://github.com/thoughtbot/climate_control/releases/tag/v1.0.0

cc: @joshuaclayton

mike-burns commented 3 years ago

@joshuaclayton the CI failures on here look like syntax errors in climate_control. Are they OK?

geoffharcourt commented 3 years ago

Sorry, Travis didn't run promptly when I opened this PR, and that led to not seeing the failures. I'll take a stab at fixing the ones that aren't for EOLed Rubies.

geoffharcourt commented 3 years ago

Update: these Travis failures are also happening on master.

geoffharcourt commented 3 years ago

I think the tests don't work in recent RSpec versions. I've tried a new branch to move to rspec-mocks and the expect syntax and got almost all of the failures resolved but still getting some weird results that look like stubbing carrying over between examples.

joshuaclayton commented 3 years ago

This is likely due to an issue with newer versions of climate control, and that I missed adding a minimum Ruby version on it (to 2.5.0) for the 1.0 release.

I don't think we should yank the 1.0 release, and instead cut 1.1 with a minimum Ruby dependency and lock this to pre-1.0 releases. What do you think?

mike-burns commented 3 years ago

Makes sense to me.

I think this branch is good to merge with climate control CI failures. I'll do that on Friday.

geoffharcourt commented 3 years ago

@joshuaclayton I do not think this is related to Ruby versions! Terrapin fails on the master branch for all Ruby versions. I think it's because the testing dependencies are somewhat loose and it's been over three years since there's been a merged commit.

geoffharcourt commented 2 years ago

@mike-burns @joshuaclayton An update here after a long lapse. I can get this branch to pass locally in Ruby 2.6.6. The CI setup is defunct now that Travis has stopped doing OSS.

I think this is worth merging as-is due to the limitations that the version lock places on projects that use Terrapin and also use Climate Control for their own testing.

mike-burns commented 2 years ago

I think this is worth merging as-is due to the limitations that the version lock places on projects that use Terrapin and also use Climate Control for their own testing.

Makes sense to me. Since CI is failing: do you agree, @joshuaclayton ?

joshuaclayton commented 2 years ago

@mike-burns yikes – broken CI doesn't feel great to me. I spun up a quick PR https://github.com/thoughtbot/terrapin/pull/16 to attempt to maintain a CI process. Do we have a set of Rubies we want to explicitly support? I'd lean towards us getting CI working, remove Travis, and rebase that work in here to get a green build.

joshuaclayton commented 2 years ago

@geoffharcourt out of curiosity, how were you able to get this running on different Ruby 2.6.6? I've got 29 failures locally on 2.6.6, which look to be replicated in this CI run as well.

After tinkering with CI, it looks much more difficult and seems we're dealing with significant code rot. There's a good number of reverse dependencies, even beyond paperclip, so it's unclear beyond getting this merged what next steps should be.

geoffharcourt commented 2 years ago

@joshuaclayton I must have made some really sloppy mistake, I'm also unable to get this working locally. The laptop I used just got wiped and traded in, so I can't recheck my work that I had at the time. Sorry for the overconfident assertion that we were GTG.

joshuaclayton commented 2 years ago

@geoffharcourt no worries – I did the same thing last week with unused! It happens 😸

rocket-turtle commented 1 year ago

@geoffharcourt I think you had an older Version of mocha on your maschine and there for the tests were green. Here is an pull request to fix the mocha version. https://github.com/thoughtbot/terrapin/pull/17/files I hope this gets this Pull request moving again.

thomasdarde commented 1 year ago

Hi ! Any news on this subject, is terrapin seeing a minimum of maintenance ? It seems all the PR are on hold, Let us know so we can help ?

geoffharcourt commented 1 year ago

Just rebased from @rocket-turtle's spec fixes, I think CI is no longer set up here.

elisuh commented 1 year ago

Hello! I'm giving terrapin some love this week. Thank you for all of your patience and help @geoffharcourt and the kind offer @thomasdarde - we appreciate you!

elisuh commented 1 year ago

@geoffharcourt It looks like the updated version of climate control breaks CI on Ruby < 2.5 - any thoughts on how you'd want to move forward here?

geoffharcourt commented 1 year ago

Hi @elisuh I took a brief look at this after the last PR merged and I wasn't 100% sure what to do next. I won't be able to look at this for at least a week. If you or someone else wants to push on this PR or a separate one that's OK with me!

thomasdarde commented 1 year ago

I think this PR is ready now that old rubies are not in the test matrix anymore

geoffharcourt commented 1 year ago

@thomasdarde I'm getting a lot of failures on Ruby 3.x that I haven't had time to work on.

geoffharcourt commented 8 months ago

I'm going to close this PR because I'm not making progress and I don't want to discourage someone else from fixing this and thinking I'm in the way.