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

feat(ruby): upgrade versions & dependencies #1067

Merged
merged 7 commits into from
Sep 18, 2024
Merged

Conversation

AndriiAndreiev
Copy link
Collaborator

@AndriiAndreiev AndriiAndreiev commented Aug 21, 2024

🚥 Resolves #1050, #1048

🧰 Changes

  • Dropped support for Ruby 2.7 and 3.0
  • Added support for Ruby 3.2
  • Added support for Ruby 3.3
  • Upgraded dependencies
  • Fixed failing tests
  • Re-enabled Ruby for Dependabot

@@ -134,5 +128,17 @@ def host_header
'Host' => @request.host
}.compact
end

def read_body(io)
Copy link
Member

Choose a reason for hiding this comment

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

@AndriiAndreiev what's the context behind this method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In short, the logic under the hood changed, and it stopped working the way it used to. The problem appears when we have empty request bodies. In this case, we call the “read” method from nil, which leads to errors. And because of this, 2 tests stopped working.
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't find any exact solutions for this, but this method seems to fix it

Copy link
Member

Choose a reason for hiding this comment

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

@AndriiAndreiev can you add a test case for this? Looks good to me, but I'd love some additional confidence in our code base.

@gratcliff gratcliff added the dependencies Pull requests that update a dependency file label Aug 21, 2024
Copy link
Member

@erunion erunion left a comment

Choose a reason for hiding this comment

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

echoing that it would be nice to see a test case added for the body reading changes but otherwise this looks good

# commit-message:
# prefix: chore(deps)
# prefix-development: chore(deps-dev)
- package-ecosystem: bundler
Copy link
Contributor

@ilias-t ilias-t Sep 3, 2024

Choose a reason for hiding this comment

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

nit: might be mistaken, but I don't believe this is related to the other changes. If not, given we squash and merge commits from PRs, maybe let's move this to a separate PR so we have a more distinct commit history post-merge.

@gratcliff
Copy link
Member

@AndriiAndreiev can you resolve conflicts here? Happy to merge after.

@gratcliff gratcliff merged commit 6139934 into main Sep 18, 2024
45 checks passed
@gratcliff gratcliff deleted the feat/ruby-upgrade branch September 18, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: support latest versions of Ruby
4 participants