openedx / cs_comments_service

server side of the comment service
GNU Affero General Public License v3.0
36 stars 155 forks source link

chore: upgrade to ruby 3 #392

Closed xitij2000 closed 1 year ago

xitij2000 commented 2 years ago

This PR adds support for running cs_comments_service on Ruby 3 in addition to Ruby 2.5

Given that Ruby 2.5 is EOL, 2.7 will be EOL within months. Ruby 3 gives us a bit over a year to upgrade to Ruby 3.1.

Sandbox URL: TBD - sandbox is being provisioned.

Merge deadline: "None"

Testing instructions:

This PR can be used with the corresponding devstack PR to manually test this PR.

Testing should involve manual interaction with all the aspects of the forum.

Author notes and concerns:

The code has been tested and is working with 3.1 as well, but there are issues when running 3.1 on CI. It will hopefully not require much effort to upgrade to 3.1

openedx-webhooks commented 2 years ago

Thanks for the pull request, @xitij2000!

As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion.

codecov[bot] commented 2 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@063fd26). Click here to learn what that means. Patch has no changes to coverable lines.

:exclamation: Current head 712a8bd differs from pull request most recent head afeb30c. Consider uploading reports for the commit afeb30c to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #392 +/- ## ========================================= Coverage ? 96.08% ========================================= Files ? 58 Lines ? 4522 Branches ? 0 ========================================= Hits ? 4345 Misses ? 177 Partials ? 0 ``` Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

natabene commented 2 years ago

@xitij2000 Thank you for the contribution, let me know once this is ready.

ghassanmas commented 2 years ago

@xitij2000 Following our today BTR call, we were disucssing whether discussion (backend service) should run on Ruby 3 or not.

One factor obviously is deadline of this PR, can you by any chance estimae when this would be ready/merged?

Also can you confirm that the alternative is to run the service on a ruby version (2.5.7) ref which already has reached its EOL?

Note: I personally tested this locally but I had to do #395 first and the test was run tutor based using the forum plugin, https://github.com/overhangio/tutor-forum/pull/11 and we are in the process of testing in the acutal production.

jmbowman commented 2 years ago

It looks like there's also a Ruby 2.7.6 upgrade option that might be easier, but that would also go EOL about 3 months before Palm is released. I think this one slipped under the radar because our Ruby entries in the support windows doc were outdated.

xitij2000 commented 2 years ago

@xitij2000 Following our today BTR call, we were disucssing whether discussion (backend service) should run on Ruby 3 or not.

One factor obviously is deadline of this PR, can you by any chance estimae when this would be ready/merged?

Also can you confirm that the alternative is to run the service on a ruby version (2.5.7) ref which already has reached its EOL?

Note: I personally tested this locally but I had to do #395 first and the test was run tutor based using the forum plugin, overhangio/tutor-forum#11 and we are in the process of testing in the acutal production.

I will try to prioritise this in the upcoming sprint. Initially the aim was just to get tests running on Ruby 3.0 to catch any new failures, and slowly fix things over time, but I can put some of my CC time to getting this running.

@jmbowman I aimed for 3.x for that very reason, that 2.7 was also close to EOL. I will see if I can get all tests passing and if the forum is running without issues on 3.x, and if not I'll see if 2.7 is easier.

mtyaka commented 2 years ago

@xitij2000 This looks great and it required less changes than I thought it would! I think we'll want to pull in the change from https://github.com/openedx/cs_comments_service/pull/395 before merging, but other than that everything looks good and seems to be working well (I tested it on The Olive demo instance) which is already running code from this branch on ruby 3).

xitij2000 commented 2 years ago

I've got all the tests running and passing on both Ruby 3.0 and 3.1, however with Ruby 3.1 the CI seems to fail due to failed dependencies when building an extension. I might not have time to resolve that, but it is working otherwise.

mtyaka commented 1 year ago

Thanks a lot for taking care of this @xitij2000! :+1:

arbrandes commented 1 year ago

@asadazam93, is this something you could look at in the next couple of days? We want to have it in as part of the Olive release, since Ruby < 3 is out of support (and has been for a while).

asadazam93 commented 1 year ago

@xitij2000 Is this a breaking change?

xitij2000 commented 1 year ago

@xitij2000 Is this a breaking change?

It should not be. This PR is tested against both the current version of Ruby (2.5.7) and on Ruby 3.0.x. However, the idea is to eventually start using Ruby 3.0, so it sets the stage for that.

ghassanmas commented 1 year ago

Hi folks, Would it be possible for this to be merged today or by tomorrow at least. Since then it need to be backported to olive and then tested again with the recent commits to olive...etc. Thus the sooner this is merged the less urguent things would need to be done for the olive relaese.

I appercaite your understaning and happy to help if anything is needed with this.

openedx-webhooks commented 1 year ago

@xitij2000 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.