Skip to content

feat: Enhance GitHub checks validation logic and add deployment gate options#58

Merged
Akanshu-2u merged 14 commits intomasterfrom
aaich/BOMS-242
Mar 20, 2026
Merged

feat: Enhance GitHub checks validation logic and add deployment gate options#58
Akanshu-2u merged 14 commits intomasterfrom
aaich/BOMS-242

Conversation

@Akanshu-2u
Copy link
Copy Markdown
Contributor

@Akanshu-2u Akanshu-2u commented Feb 27, 2026

Issue Description:

Deployment gate was incorrectly allowing deployments to proceed while GitHub CI checks were still running. This happened because required checks that haven't been created yet (like "Unit tests successful" which waits for individual test shards) don't exist in GitHub's API response and were being ignored, allowing the deployment gate to pass prematurely.

Root Cause:

In get_validation_results(), we only validated checks that existed in the GitHub API response. If a required check hadn't been created yet (still waiting for dependent jobs), it simply wasn't in the results dictionary, so the deployment gate considered validation complete and passed.

Solution:

Added a simple check at the end of get_validation_results() to ensure ALL required checks are accounted for:

# If a required check is missing from the results, add it as pending to block deployment.
# This ensures we wait for ALL required checks to be created and pass.
if not self.all_checks and required_checks:
    for required_check in required_checks:
        if required_check not in results:
            results[required_check] = ('pending', None)

What this does:

  • After collecting all checks from GitHub API (statuses, check suites, check runs)
  • Loop through the required checks from branch protection rules
  • For any required check that's missing from results, add it with status 'pending'
  • This blocks deployment until the check appears and passes
  • This prevents deployments from proceeding while CI checks are still running.

Testing:

Created comprehensive test script
test_deployment_gate.sh
that replicates the GoCD pipeline locally and validates the fix works correctly on edx/edx-platform commit CI runs.

Instructions to run the script locally:

  • export GITHUB_TOKEN=your_github_personal_access_token
  • Basic usage (tests default edx-platform/release-ulmo):
    ./test_deployment_gate.sh
  • Test specific commit:
    ./test_deployment_gate.sh <commit_sha>
  • Test with your fix branch:
    TUBULAR_BRANCH=<your-branch> ./test_deployment_gate.sh

Result:

  1. On required checks pending:
Screenshot 2026-03-12 172748

2 On any required check successful:
Screenshot 2026-03-12 172821

Private JIRA Link:

BOMS-242

Copilot AI review requested due to automatic review settings February 27, 2026 12:46
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR strengthens CI/check validation for GitHub commits/PRs and introduces deployment-gate-oriented options to reduce race conditions where checks haven’t started or are still running.

Changes:

  • Add --min-checks and --fail-on-pending options to check_pr_tests_status.py and expand per-check status diagnostics.
  • Improve GitHubAPI.get_validation_results() state mapping to consider check-suite/check-run status when conclusion is missing, and harden URL/state handling.
  • Adjust aggregation logic and commit polling internals to better distinguish “no checks found” from other outcomes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File Description
tubular/scripts/check_pr_tests_status.py Adds deployment gate controls (min_checks, pending handling) and more detailed check outcome reporting.
tubular/github_api.py Enhances validation result extraction/aggregation across statuses, check suites, and check runs; adds logging and a “no checks” sentinel.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tubular/scripts/check_pr_tests_status.py Outdated
Comment thread tubular/scripts/check_pr_tests_status.py Outdated
Comment thread tubular/scripts/check_pr_tests_status.py
Comment thread tubular/github_api.py Outdated
Comment thread tubular/github_api.py Outdated
Comment thread tubular/github_api.py Outdated
Comment thread tubular/scripts/check_pr_tests_status.py
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tubular/scripts/check_pr_tests_status.py
Comment thread tubular/scripts/check_pr_tests_status.py Outdated
Comment thread tubular/scripts/check_pr_tests_status.py
Comment thread tubular/github_api.py
Comment thread tubular/github_api.py Outdated
Comment thread tubular/github_api.py Outdated
Copilot AI review requested due to automatic review settings March 3, 2026 12:45
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tubular/github_api.py Outdated
Comment thread tubular/github_api.py Outdated
Comment thread tubular/scripts/check_pr_tests_status.py Outdated
Comment thread tubular/tests/test_github.py Outdated
Comment thread tubular/scripts/check_pr_tests_status.py Outdated
Copilot AI review requested due to automatic review settings March 6, 2026 13:08
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tubular/github_api.py Outdated
Comment thread tubular/github_api.py Outdated
Comment thread tubular/github_api.py Outdated
Copilot AI review requested due to automatic review settings March 6, 2026 13:54
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings March 12, 2026 18:28
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread tubular/github_api.py
Comment thread tubular/github_api.py Outdated
Comment thread tubular/github_api.py
Copilot AI review requested due to automatic review settings March 13, 2026 09:49
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread tubular/github_api.py
@Akanshu-2u Akanshu-2u requested a review from timmc-edx March 13, 2026 13:30
@timmc-edx
Copy link
Copy Markdown
Member

This looks fine, but would you mind updating the PR description to reflect the current state of the code? It looks like you had added a lot of code and tests but eventually stripped it back down to something much simpler.

@Akanshu-2u
Copy link
Copy Markdown
Contributor Author

This looks fine, but would you mind updating the PR description to reflect the current state of the code? It looks like you had added a lot of code and tests but eventually stripped it back down to something much simpler.

The PR description currently describes the current solution which is the stripped one. So maybe it does not require any modifications.

@timmc-edx
Copy link
Copy Markdown
Member

Ah, sorry, I misread -- the extra output ("DEPLOYMENT GATE PASSED" etc.) was moved from the PR branch into the attached shell script. That's fine, then.

I haven't looked at the shell script in any detail. It appears to be a test harness for the changes. However, we should instead add a test case to tubular's existing unit tests -- this will ensure that the changes work not just now, but also in the future when this code is updated for other reasons.

@Akanshu-2u
Copy link
Copy Markdown
Contributor Author

Ah, sorry, I misread -- the extra output ("DEPLOYMENT GATE PASSED" etc.) was moved from the PR branch into the attached shell script. That's fine, then.

I haven't looked at the shell script in any detail. It appears to be a test harness for the changes. However, we should instead add a test case to tubular's existing unit tests -- this will ensure that the changes work not just now, but also in the future when this code is updated for other reasons.

I have added unit tests handling all the situations for the current fix. Thank you.

Comment on lines +433 to +436
['Unit tests successful'],
[],
{},
True,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm. Wouldn't we want required, missing checks to be included as pending here? (I had missed the all_checks part during my initial review.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, this behavior is intentional.

  • When all_checks=True, the goal is just to show all checks that actually exist for visibility or reporting.

    → So, we do NOT add any fake pending checks, because that would give a misleading picture.

  • When all_checks=False, the goal is to validate required checks before deployment.

    → In this case, if any required checks are missing, we add them as “pending” to ensure deployment gets blocked.

As the deployment gate uses all_checks=False , so the fix and the tests works.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks. Do you know if that's documented anywhere, or are you just inferring it from the code?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That looks like something you wrote describing this particular ticket and PR. What I'm wondering is what your original source is for information about all_checks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The original source for the information about all_checks is this commit: c2add71

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you just mean the click.option arg help="Check all validation contexts, whehther it is required or not", or is there something else there that I'm not seeing?

@Akanshu-2u Akanshu-2u requested a review from timmc-edx March 18, 2026 06:40
@Akanshu-2u Akanshu-2u merged commit 72b4fc1 into master Mar 20, 2026
3 checks passed
@Akanshu-2u Akanshu-2u deleted the aaich/BOMS-242 branch March 20, 2026 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants