search

oppia / oppia

A free, online learning platform to make quality education accessible for all.
https://www.oppia.org
Apache License 2.0
5.63k stars 3.79k forks source link

Migrate CI to Docker #19962

Closed gp201 closed 1 week ago

gp201 commented 2 months ago

Overview

  1. This PR fixes or fixes part of #[fill_in_number_here].
  2. This PR does the following: [Explain here what your PR does and why]
  3. (For bug-fixing PRs only) The original bug occurred because: [Explain what the cause of the bug was, and which PR introduced it]

Essential Checklist

Testing doc (for PRs with Beam jobs that modify production server data)

Proof that changes are correct

Proof of changes on desktop with slow/throttled network

Proof of changes on mobile phone

Proof of changes in Arabic language

PR Pointers

oppiabot[bot] commented 2 months ago

Hi @gp201, can you complete the following:

  1. The body of this PR is missing the required description, please update the body with a description of what this PR does.
  2. The karma and linter checklist has not been checked, please make sure to run the frontend tests and lint tests before pushing. Thanks!
oppiabot[bot] commented 2 months ago

Hi @gp201 please assign the required reviewer(s) for this PR. Thanks!

oppiabot[bot] commented 2 months ago

Hi @gp201. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

oppiabot[bot] commented 2 months ago

Hi @gp201, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

jayam04 commented 2 months ago

@gp201 PTAL at comment, need your response before I can move forward on it.

jayam04 commented 1 month ago

@seanlip, @StephenYu2018 Could you review implementation of fixing google module for docker made in commit here. I will add backend tests if approved.

An alternative would be to call install_third_party_libs.main() explicitly and handling docker-specific instructions in install_third_party_libs.py

oppiabot[bot] commented 1 month ago

Unassigning @StephenYu2018 since the review is done.

StephenYu2018 commented 1 month ago

@jayam04 I believe there is still one prior concern about a browser.pause statement in an E2E test file that hasn't been addressed yet. It is the only thing stopping me from approving this PR.

jayam04 commented 1 month ago

@jayam04 I believe there is still one prior concern about a browser.pause statement in an E2E test file that hasn't been addressed yet. It is the only thing stopping me from approving this PR.

@StephenYu2018 I know it, currently focusing on other bugs due to which some e2e tests are failing. Once done, I will update browser.pause appropriate solution.

oppiabot[bot] commented 1 month ago

Assigning @U8NWXD, @Lawful2002 for code owner reviews. Thanks!

jayam04 commented 1 month ago

@seanlip replied to review here.

jayam04 commented 1 month ago

@seanlip, @StephenYu2018 Some tests are still failing, should I use browser.pause() over there too and create a new issue for same.

seanlip commented 1 month ago

@U8NWXD @Lawful2002 PTAL as codeowners, thanks.

jayam04 commented 1 month ago

@seanlip Created new issue for one e2e test failing. Could you review issue, do I need to add more info? Once done, I will open issues for other 5 failing tests.

seanlip commented 1 month ago

Thanks @jayam04 -- the issue looks good, but I have some concerns about merging this with 6 known flakes that are likely to keep cropping up and blocking developers from merging things. It's going to make the dev workflow very tedious if developers have to keep rerunning the tests again and again.

I think it'd still be good to file the issues now, but after that, could you work with folks to try and fix them before we merge this PR? @jnvtnguyen has been doing some flake fixes recently, and might be a good person to ask for tips/help.

Thanks!

gp201 commented 1 month ago

I have addressed some of the comments. Will address the remaining by tomorrow:

  1. https://github.com/oppia/oppia/pull/19962#discussion_r1577024885
  2. https://github.com/oppia/oppia/pull/19962#discussion_r1579864271
  3. https://github.com/oppia/oppia/pull/19962#discussion_r1579873250
  4. https://github.com/oppia/oppia/pull/19962#discussion_r1579876645
  5. https://github.com/oppia/oppia/pull/19962#discussion_r1579880172
  6. https://github.com/oppia/oppia/pull/19962#discussion_r1579887478
oppiabot[bot] commented 4 weeks ago

Unassigning @Lawful2002 since they have already approved the PR.

gp201 commented 4 weeks ago

Hi @U8NWXD,

Apologies for the trouble, and thanks for your patience. Since this PR is time-sensitive, could you quickly check out the other comments?

Right now, I'm focusing on resolving the comments provided in these two discussions:

  1. https://github.com/oppia/oppia/pull/19962#discussion_r1577024885
  2. https://github.com/oppia/oppia/pull/19962#discussion_r1579864271
U8NWXD commented 3 weeks ago

I've replied to or resolved my responded-to comments

oppiabot[bot] commented 2 weeks ago

Hi @gp201, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!