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

Add ostruct to Gemfile for Ruby >= 3.3.5 #3890

Merged
merged 3 commits into from
Sep 5, 2024
Merged

Conversation

y9v
Copy link
Member

@y9v y9v commented Sep 4, 2024

This addresses the deprecation warning that ostruct will not be part of standard library since ruby 3.5

What does this PR do?
This PR adds ostruct gem to Gemfile for Ruby version 3.3.5 and higher.

Motivation:
Failing tests due to a deprecation warning:
https://github.com/DataDog/dd-trace-rb/actions/runs/10703793866/job/29675192240?pr=3887

@y9v y9v self-assigned this Sep 4, 2024
@y9v y9v requested a review from a team as a code owner September 4, 2024 18:07
@y9v y9v marked this pull request as draft September 4, 2024 18:14
@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.86%. Comparing base (ae50107) to head (f4c0c4e).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3890   +/-   ##
=======================================
  Coverage   97.85%   97.86%           
=======================================
  Files        1277     1277           
  Lines       76392    76392           
  Branches     3744     3744           
=======================================
+ Hits        74757    74764    +7     
+ Misses       1635     1628    -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pr-commenter
Copy link

pr-commenter bot commented Sep 4, 2024

Benchmarks

Benchmark execution time: 2024-09-05 10:32:41

Comparing candidate commit f4c0c4e in PR branch fix-ostruct-deprecation-warning with baseline commit ae50107 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 23 metrics, 2 unstable metrics.

Gemfile Outdated
@@ -62,6 +62,8 @@ if RUBY_VERSION >= '2.6.0'
gem 'rubocop-rspec', ['~> 2.20', '< 2.21'], require: false
end

gem 'ostruct' if RUBY_VERSION >= '3.3.5'
Copy link
Member

Choose a reason for hiding this comment

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

Minor: I suggest maybe doing >= 3.3 instead of 3.3.5.

My thinking is that we're not very consistent in upgrading to point releases in our different test/development environments, and on the off chance we have some weird issue from this, it may be a bit harder to diagnose if only some of the 3.3 series gets this new version.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed here 261b590 (force pushed)

@y9v y9v force-pushed the fix-ostruct-deprecation-warning branch 2 times, most recently from fd8bc2a to ea38b4d Compare September 5, 2024 08:29
This addresses the deprecation warning that `ostruct` will not be
part of standard library since ruby 3.5
@y9v y9v force-pushed the fix-ostruct-deprecation-warning branch from ea38b4d to 4e51340 Compare September 5, 2024 08:30
@y9v y9v marked this pull request as ready for review September 5, 2024 08:32
@ivoanjo
Copy link
Member

ivoanjo commented Sep 5, 2024

This looks good... But there's one more missing piece. Specifically, this will affect the appraisal gemfiles for 3.3 rubies.

Supposedly we have a Github Action that should trigger to update this, but the trigger condition is buggy (found this recently) and sometimes doesn't trigger when it should (e.g. in this PR): https://github.com/DataDog/dd-trace-rb/actions/runs/10716765010/job/29715426163 .

As a quick workaround, I've opened another PR (#3892) to force the GHA to trigger: https://github.com/DataDog/dd-trace-rb/actions/runs/10717020845/job/29715869065 .

It's currently running, and once it's done, we can cherry-pick that change on top of this PR.

@y9v
Copy link
Member Author

y9v commented Sep 5, 2024

@ivoanjo thanks! I haven't thought about this.

Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@ivoanjo
Copy link
Member

ivoanjo commented Sep 5, 2024

@ivoanjo thanks! I haven't thought about this.

No worries! Our setup is quite complex to try to cover all the testing we need, and it has a few gaps that we haven't yet plugged :)

I've cherry-picked the commit generated by GHA, I think we're good to go once CI is green and beautiful :)

@y9v y9v merged commit 5f6f052 into master Sep 5, 2024
189 of 191 checks passed
@y9v y9v deleted the fix-ostruct-deprecation-warning branch September 5, 2024 10:34
@TonyCTHsu TonyCTHsu added the dev/internal Other internal work that does not need to be included in the changelog label Sep 6, 2024
TonyCTHsu added a commit that referenced this pull request Sep 6, 2024
…-warning"

This reverts commit 5f6f052, reversing
changes made to ae50107.
TonyCTHsu added a commit that referenced this pull request Sep 6, 2024
…-warning"

This reverts commit 5f6f052, reversing
changes made to ae50107.
@anmarchenko anmarchenko added this to the 2.4.0 milestone Sep 9, 2024
p-datadog pushed a commit to p-datadog/dd-trace-rb that referenced this pull request Sep 9, 2024
* master:
  add missing permissions for add-milestone gha workflow
  Improve brittle assertion
  Revert "Merge pull request DataDog#3890 from DataDog/fix-ostruct-deprecation-warning"
  fix lint error
  test for patched tracer_version
  add a comment about tracer_version patch
  fix typechecks and replace + with - in extended version
  add ci-X.Y.Z to the tracer_version if datadog-ci is present and CI mode is enabled
y9v pushed a commit that referenced this pull request Sep 13, 2024
…-warning"

This reverts commit 5f6f052, reversing
changes made to ae50107.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev/internal Other internal work that does not need to be included in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants