Skip to content

build: run github actions on pull requests - #27786

Merged
mraarif merged 2 commits into
openedx:masterfrom
regisb:regisb/pr-github-actions
Oct 4, 2021
Merged

build: run github actions on pull requests#27786
mraarif merged 2 commits into
openedx:masterfrom
regisb:regisb/pr-github-actions

Conversation

@regisb

@regisb regisb commented May 31, 2021

Copy link
Copy Markdown
Contributor

Description

This will run mypy within GitHub actions for every pull request.

Currently, some types are annotated within edx-platform, but they are not actively enforced. This change will make sure that mypy is run for every PR.

Supporting information

See the corresponding discussion: https://discuss.openedx.org/t/dev-notes-running-mypy-on-edx-platform/4860/2
And the previous PR: https://github.com/edx/edx-platform/pull/27575

Other information

Note that running GitHub Actions for every PR might make us hit the usage limits, as installing python requirements is a costly operation.

@openedx-webhooks

openedx-webhooks commented May 31, 2021

Copy link
Copy Markdown

Thanks for the pull request, @regisb! I've created OSPR-5818 to keep track of it in JIRA, where we prioritize reviews. 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.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels May 31, 2021
@regisb
regisb force-pushed the regisb/pr-github-actions branch 3 times, most recently from 83d0f16 to 86966a2 Compare May 31, 2021 13:32
@regisb

regisb commented May 31, 2021

Copy link
Copy Markdown
Contributor Author

This is ready for review.

@natabene

natabene commented Jun 1, 2021

Copy link
Copy Markdown
Contributor

@regisb Thank you for your contribution. Is this ready for a review?

@natabene natabene added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Jun 1, 2021
Comment thread .github/workflows/test.yml Outdated
Comment thread .github/workflows/test.yml Outdated
Comment thread .github/workflows/test.yml Outdated
Comment thread .github/workflows/test.yml Outdated
@arch-bom-gocd-alerts

Copy link
Copy Markdown

📣 💥 Heads-up: You must either rebase onto master or merge master into your branch to avoid breaking the build.

We recently removed diff-quality and introduced lint-amnesty. This means that the automated quality check that has run on your branch doesn't work the same way it will on master. If you have introduced any quality failures, they might pass on the PR but then break the build on master.

This branch has been detected to not have commit 2e33565 as an ancestor. Here's how to see for yourself:

git merge-base --is-ancestor 2e335653 regisb/pr-github-actions && echo "You're all set" || echo "Please rebase onto master or merge master to your branch"

If you have any questions, please reach out to the Architecture team (either #edx-shared-architecture on Open edX Slack or #architecture on edX internal).

@natabene

Copy link
Copy Markdown
Contributor

@regisb Just checking, have you had a chance to implement the feedback provided?

@regisb

regisb commented Jul 19, 2021

Copy link
Copy Markdown
Contributor Author

@natabene not yet! I did not have time during the past couple weeks and I'm currently on holidays. I will get back to this PR after I return.

@natabene natabene removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jul 19, 2021
@regisb
regisb force-pushed the regisb/pr-github-actions branch from 86966a2 to 24edc5c Compare August 30, 2021 14:57
@regisb

regisb commented Aug 30, 2021

Copy link
Copy Markdown
Contributor Author

This is ready for review again :)

Comment thread .github/workflows/ci-static-analysis.yml Outdated
@regisb
regisb force-pushed the regisb/pr-github-actions branch 2 times, most recently from 64c4918 to a87fd2f Compare September 9, 2021 20:49
@regisb

regisb commented Sep 9, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for your review @mraarif! I made the suggested changes. I also managed to test CI with act:

act --reuse --workflows .github/workflows/ci-static-analysis.yml "pull_request"

I also added a typing fix such that CI can actually pass without error.

For now, the only task that is performed is type-checking with mypy.

This task was tested with act (https://github.com/nektos/act):

    act --reuse --workflows .github/workflows/ci-static-analysis.yml "pull_request"
@regisb
regisb force-pushed the regisb/pr-github-actions branch from a87fd2f to 1d685f2 Compare September 9, 2021 22:11
@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests. There were no failures.

@jmbowman jmbowman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, thanks for setting this up!

@mraarif
mraarif merged commit 0af38bf into openedx:master Oct 4, 2021
@openedx-webhooks

Copy link
Copy Markdown

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

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@timmc-edx

Copy link
Copy Markdown
Contributor

FYI, one error snuck in on master while this PR was open:

openedx/core/djangoapps/content/learning_sequences/api/processors/enrollment_track_partition_groups.py:31: error: Need type annotation for "enrollment_track_groups" (hint: "enrollment_track_groups: Dict[<type>, <type>] = ...")

I think Arbi-BOM is going to take care of fixing that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants