-
Notifications
You must be signed in to change notification settings - Fork 155
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
bump: add ddtrace in Gemfile #429
Conversation
Thanks for the pull request, @ghassanmas! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
@ghassanmas The ruby CI is failing on this. Can you please check why? |
@feanil Hi, it seems the CI is failing due to the missing codecov token. |
Yes it same issue with other reop once fixed I can rebase |
I've added the token, you should be able to update the workflow and get coverage working again. |
I am not sure if what I did is the correct way to update the workflow.... orginally this repo relied on codecov 0.6..0 gem, which is already deprecated, the readme recommend to use github action CI direclty... |
e0d7f1d
to
80994d0
Compare
Hi @ghassanmas, thank you for this contribution! @asadazam93 @DawoudSheraz Would you have any tips for Ghassan about how to fix the failing build here? |
Hi. Unfortunately, I don't have context on this. Tagging @feanil as he might have some context for this. Thanks |
@ghassanmas I'm taking a look at this. I think actually ddtrace should not have been added as an explicit requirement so I'm digging a bit further. |
@@ -45,7 +45,7 @@ gem 'dalli' | |||
gem 'rest-client' | |||
|
|||
group :test do | |||
gem 'codecov', :require => false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
install newer version of codecov for ruby, as I remeber I followed this doc https://docs.codecov.com/docs/deprecated-uploader-migration-guide#ruby-uploader
Since it was using the deprecated uploader https://github.com/codecov/codecov-ruby?tab=readme-ov-file
@@ -69,3 +69,5 @@ jobs: | |||
- name: Run tests | |||
run: bin/rspec -fd | |||
continue-on-error: ${{ matrix.allow-failure }} | |||
- name: Send test coverage report to codecov.io | |||
uses: codecov/codecov-action@v4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Action v4 will also need to make use of the codecov secret when not working with PRs from forks.
This exists in this repo as secrets.CODECOV_TOKEN
but you'll need to make use of it here similar to here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will try this out right now
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #429 +/- ##
=======================================
Coverage 96.19% 96.19%
=======================================
Files 58 58
Lines 4624 4624
=======================================
Hits 4448 4448
Misses 176 176 ☔ View full report in Codecov by Sentry. |
@feanil now that the builds is success, shall this await for |
I think it's fine to have dd-trace in here for now and we'll follow up on it separately. We can probably just leave it since it's not hurting anything and we're potentially investing in a replacement app soon. |
@ghassanmas 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Ths change add
ddtrace
, which is used incs_comments_service/config/unicorn_tcp.rb
Line 15 in 8626d44
Related issue: