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

Switch from WEBrick to Puma #112

Merged
merged 5 commits into from
Nov 18, 2024
Merged

Switch from WEBrick to Puma #112

merged 5 commits into from
Nov 18, 2024

Conversation

tschafer-gc
Copy link
Contributor

WEBrick appears to be approaching EOL and is being removed from Rackup, as mentioned here. This is causing issues with a number of services as discussed here. By switching to Puma we can solve this issue while also still supporting both Rack 2 and 3.

@tschafer-gc tschafer-gc force-pushed the tschafer-switch-to-puma branch from 1f6c099 to 1702563 Compare November 8, 2024 16:24
@tschafer-gc tschafer-gc force-pushed the tschafer-switch-to-puma branch from 1702563 to a4b5040 Compare November 8, 2024 17:28
@tschafer-gc tschafer-gc force-pushed the tschafer-switch-to-puma branch from 5dcbee3 to 529392f Compare November 8, 2024 22:58
@tschafer-gc tschafer-gc marked this pull request as ready for review November 13, 2024 09:58
@stephenbinns
Copy link
Contributor

Looks good but can we update the smoke test so it runs both Rack 2 and 3 - https://github.com/gocardless/que/blob/master/.github/workflows/tests.yml#L22-L57

@tschafer-gc tschafer-gc force-pushed the tschafer-switch-to-puma branch 2 times, most recently from 8523a09 to bca0dd5 Compare November 14, 2024 10:13
@tschafer-gc
Copy link
Contributor Author

Looks good but can we update the smoke test so it runs both Rack 2 and 3 - https://github.com/gocardless/que/blob/master/.github/workflows/tests.yml#L22-L57

@stephenbinns I've just updated the smoke tests - does that look okay? My implementation feels a bit hacky but I'm not sure of a better way to ensure it uses both Rack versions

@tschafer-gc tschafer-gc force-pushed the tschafer-switch-to-puma branch from bca0dd5 to 5fcf274 Compare November 14, 2024 10:23
@stephenbinns
Copy link
Contributor

stephenbinns commented Nov 14, 2024

Looks good but can we update the smoke test so it runs both Rack 2 and 3 - https://github.com/gocardless/que/blob/master/.github/workflows/tests.yml#L22-L57

@stephenbinns I've just updated the smoke tests - does that look okay? My implementation feels a bit hacky but I'm not sure of a better way to ensure it uses both Rack versions

Makes sense, as for making it a bit less hacky you should be able to put a conditional in the Gemfile to achieve the same result

if ENV['RACK_VERSION'] == '2.0'
  gem "rack", "~> 2.0"
else
  gem "rack", "~> 3.0"
end

And then pass the context in the env for the github action

    env:
      RACK_VERSION: "${{ matrix.rack-version }}"

@tschafer-gc tschafer-gc force-pushed the tschafer-switch-to-puma branch 2 times, most recently from 31ac735 to b479947 Compare November 14, 2024 11:58
Gemfile Outdated
@@ -25,6 +25,13 @@ group :test do
gem 'rspec', '~> 3.9'
end

gem "rack", ENV['RACK_VERSION']
if ENV['RACK_VERSION'] == '2.0'
Copy link

Choose a reason for hiding this comment

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

Versions can be compared using: https://stackoverflow.com/a/3064161

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 point, updated - thanks!

@tschafer-gc tschafer-gc force-pushed the tschafer-switch-to-puma branch from b479947 to 0063b34 Compare November 14, 2024 12:06
@tschafer-gc tschafer-gc force-pushed the tschafer-switch-to-puma branch 3 times, most recently from 3717bc3 to aa62ef9 Compare November 14, 2024 15:38
@tschafer-gc tschafer-gc force-pushed the tschafer-switch-to-puma branch from aa62ef9 to a2c331f Compare November 14, 2024 16:13
@tschafer-gc tschafer-gc merged commit 2487508 into master Nov 18, 2024
18 checks passed
@tschafer-gc tschafer-gc deleted the tschafer-switch-to-puma branch November 18, 2024 10:37
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.

4 participants