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

Fix edx-platform required checks #34789

Closed
kdmccormick opened this issue Mar 28, 2024 · 9 comments · Fixed by #34903
Closed

Fix edx-platform required checks #34789

kdmccormick opened this issue Mar 28, 2024 · 9 comments · Fixed by #34903
Assignees

Comments

@kdmccormick
Copy link
Member

kdmccormick commented Mar 28, 2024

Repository

axim-engineering

Urgency

Low (2 weeks)

Requested Change

The set of required checks on edx-platform and/or the "unit tests successful" check itself need to be fixed, although we're not exactly sure how yet.

They don't currently enforce that all unit tests pass, as the Unit tests successful check seems to be getting skipped.

More details:

We have disabled Auto-merge on edx-platform because of this. As part of resolving this ticket, please re-enable it.

Reasoning

.

@openedx-workflow-automation
Copy link

Thank you for your report! @openedx/axim-oncall will triage within a business day. Simple requests usually take 2-3 business days to resolve; more complex requests could take longer.

@sarina
Copy link
Contributor

sarina commented Apr 4, 2024

@kdmccormick @e0d are either of you taking this, or is this a backlog eng item?

@kdmccormick
Copy link
Member Author

I am not, and I doubt Ed is, so it's in the backlog, although I'd vouch for it to be high up in the backlog.

@sarina
Copy link
Contributor

sarina commented Apr 4, 2024

I'm just wondering if it's an on-call ticket or not. I don't know how to address this.

@kdmccormick kdmccormick changed the title [GH Request] Fix edx-platform required checks Fix edx-platform required checks Apr 4, 2024
@kdmccormick
Copy link
Member Author

Fair, I guess it's more edx-platform maintenance than on-call work.

@feanil and I are next up in the on-call queue, so I figure whichever one of us has time first can pick it up.

@kdmccormick
Copy link
Member Author

kdmccormick commented May 6, 2024

@feanil LMK here when you switch us over to github hosted unit test runners. I think it should make these required checks much easier to manage. I'm reeealllllyyyy looking forward to being able to auto-merge edx-platform PRs again 😄

@kdmccormick kdmccormick transferred this issue from openedx/axim-engineering May 13, 2024
kdmccormick added a commit that referenced this issue Jun 3, 2024
This ensures that the `success` job in unit-tests.yml
always runs, allowing us to protect master from unit
test failures by making the `success` job required.

From https://github.com/marketplace/actions/alls-green#options:

> Important: For this to work properly, it is a must to have the job always run,
> otherwise GitHub will make it skipped when any of the dependencies fail. In
> some contexts, skipped is interpreted as success which may lead to undersired,
> unobvious and even dangerous (as in security breach "dangerous") side-effects.

Closes #34789
kdmccormick added a commit that referenced this issue Jun 3, 2024
When a shard of unit-tests.yml fails, we want the `success` job to be
maked "Failed" (not "Skipped"). That's because "Failed" blocks the PR
from merging, whereas "Skipped" does not. This change ensures that
`success` always runs to completion rather than being cancelled as soon
as a unit test shard fails or is cancelled.

From https://github.com/marketplace/actions/alls-green#options:

> Important: For this to work properly, it is a must to have the job always run,
> otherwise GitHub will make it skipped when any of the dependencies fail. In
> some contexts, skipped is interpreted as success which may lead to undersired,
> unobvious and even dangerous (as in security breach "dangerous") side-effects.

Closes #34789
@kdmccormick kdmccormick moved this from Backlog to In Progress in Axim Engineering Tasks Jun 3, 2024
@kdmccormick
Copy link
Member Author

Now that we run all tests on the same infra (GH-hosted runners), this issue is much more tractable.

@feanil ready for your review: #34903

kdmccormick added a commit that referenced this issue Jun 4, 2024
When a shard of unit-tests.yml fails, we want the `success` job to be
maked "Failed" (not "Skipped"). That's because "Failed" blocks the PR
from merging, whereas "Skipped" does not. This change ensures that
`success` always runs to completion rather than being cancelled as soon
as a unit test shard fails or is cancelled.

From https://github.com/marketplace/actions/alls-green#options:

> Important: For this to work properly, it is a must to have the job always run,
> otherwise GitHub will make it skipped when any of the dependencies fail. In
> some contexts, skipped is interpreted as success which may lead to undersired,
> unobvious and even dangerous (as in security breach "dangerous") side-effects.

Closes #34789
@github-project-automation github-project-automation bot moved this from In Progress to Done in Axim Engineering Tasks Jun 4, 2024
@github-project-automation github-project-automation bot moved this to Done in Arch-BOM Jun 4, 2024
@kdmccormick
Copy link
Member Author

kdmccormick commented Jun 4, 2024

Remaining tasks:

  • do the same thing for pylint
  • wait a couple weeks (for most people to rebase), and then re-enable auto-merge

@kdmccormick
Copy link
Member Author

Pylint is already always() run: https://github.com/openedx/edx-platform/blob/master/.github/workflows/pylint-checks.yml#L75

Several weeks have gone by, so I think we're safe to re-enable auto-merge.

@github-project-automation github-project-automation bot moved this from In Progress to Done in Axim Engineering Tasks Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants