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

bump: add ddtrace in Gemfile #429

Merged
merged 2 commits into from
Jun 20, 2024
Merged

bump: add ddtrace in Gemfile #429

merged 2 commits into from
Jun 20, 2024

Conversation

ghassanmas
Copy link
Member

Ths change add ddtrace, which is used in

require 'ddtrace'

Related issue:

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 2, 2024
@openedx-webhooks
Copy link

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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@asadazam93
Copy link
Contributor

@ghassanmas The ruby CI is failing on this. Can you please check why?

@DawoudSheraz
Copy link

@feanil Hi, it seems the CI is failing due to the missing codecov token.

@ghassanmas
Copy link
Member Author

Yes it same issue with other reop once fixed I can rebase

@feanil
Copy link
Contributor

feanil commented May 6, 2024

I've added the token, you should be able to update the workflow and get coverage working again.

@ghassanmas
Copy link
Member Author

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...

@ghassanmas ghassanmas force-pushed the fix-3.3 branch 2 times, most recently from e0d7f1d to 80994d0 Compare May 7, 2024 16:19
@itsjeyd itsjeyd added the core contributor PR author is a Core Contributor (who may or may not have write access to this repo). label May 23, 2024
@itsjeyd
Copy link

itsjeyd commented May 23, 2024

Hi @ghassanmas, thank you for this contribution!

@asadazam93 @DawoudSheraz Would you have any tips for Ghassan about how to fix the failing build here?

@DawoudSheraz
Copy link

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

@itsjeyd itsjeyd requested a review from feanil May 31, 2024 07:28
@itsjeyd itsjeyd added the needs maintainer attention Issue or PR specifically needs the attention of the maintainer. label May 31, 2024
@feanil
Copy link
Contributor

feanil commented Jun 4, 2024

@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
Copy link
Contributor

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?

Copy link
Member Author

@ghassanmas ghassanmas Jun 14, 2024

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
Copy link
Contributor

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.

Copy link
Member Author

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

@itsjeyd itsjeyd added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs maintainer attention Issue or PR specifically needs the attention of the maintainer. labels Jun 6, 2024
Copy link

codecov bot commented Jun 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.19%. Comparing base (8626d44) to head (5b9b421).

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.
📢 Have feedback on the report? Share it here.

@ghassanmas
Copy link
Member Author

@feanil now that the builds is success, shall this await for ddtrace ? or is ready to be reviewed

@ghassanmas ghassanmas removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jun 14, 2024
@ghassanmas ghassanmas added needs maintainer attention Issue or PR specifically needs the attention of the maintainer. redwood and removed redwood labels Jun 14, 2024
@feanil
Copy link
Contributor

feanil commented Jun 20, 2024

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.

@feanil feanil merged commit 0900d28 into openedx:master Jun 20, 2024
16 checks passed
@openedx-webhooks
Copy link

@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.

@itsjeyd itsjeyd removed the needs maintainer attention Issue or PR specifically needs the attention of the maintainer. label Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core contributor PR author is a Core Contributor (who may or may not have write access to this repo). open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants