Ignore e2e-status on draft pr

[piotr-iohk]

Description

Ignore e2e-status on draft pr

Preview

QA Notes

[claude]

Code Review - PR #468: Ignore e2e-status on draft pr

Summary

This PR adds a draft PR check to the e2e-status job to prevent it from running on draft pull requests. The change is minimal and focused.

Code Quality ✅

Positive aspects:

• Consistency: The change aligns with the existing pattern used in the workflow. The same condition if: github.event.pull_request.draft == false is already applied to the build, e2e-branch, and e2e-tests jobs (lines 22, 64, 71).
• Clean implementation: Simple one-line addition that's easy to understand and maintain.
• Proper placement: The condition is correctly placed at the job level.

Potential Issues ⚠️

Critical: Duplicate if conditions

There's a logical error in the workflow file at .github/workflows/e2e.yml:209-214:

e2e-status:
  if: github.event.pull_request.draft == false
  name: e2e-status
  runs-on: ubuntu-latest
  needs: [e2e-tests]
  if: always()

Problem: The job now has two if conditions (lines 210 and 214). In GitHub Actions YAML, when you define the same key twice, only the last one is preserved. This means:

• The new draft check on line 210 will be overridden by if: always() on line 214
• The intended behavior (skipping on draft PRs) will not work

Solution: Combine both conditions using logical operators:

e2e-status:
  if: always() && github.event.pull_request.draft == false
  name: e2e-status
  runs-on: ubuntu-latest
  needs: [e2e-tests]

This ensures:

1. The job always runs when needed (to report status), even if e2e-tests fails
2. It skips execution on draft PRs

Performance Considerations ✅

• Resource efficiency: Skipping the status job on draft PRs saves CI resources, though the job itself is lightweight.
• Workflow behavior: The change maintains the fail-fast behavior for draft PRs by not running unnecessary status checks.

Security Concerns ✅

No security issues identified. The change doesn't introduce any security risks.

Test Coverage 📋

Recommendation: Before merging, manually verify:

1. Create a draft PR and confirm the e2e-status job doesn't run
2. Mark the PR as ready for review and confirm the e2e-status job runs correctly
3. Verify the job still runs with always() behavior when e2e-tests fails (after applying the fix)

Additional Observations

Workflow consistency: All main jobs in the E2E workflow now consistently skip draft PRs:

• build - line 22 ✅
• e2e-branch - line 64 ✅
• e2e-tests - line 71 ✅
• e2e-status - line 210 ⚠️ (needs fix)

---

Action Required

Please update line 210 to combine both conditions:

- if: github.event.pull_request.draft == false
+ if: always() && github.event.pull_request.draft == false
  name: e2e-status
  runs-on: ubuntu-latest
  needs: [e2e-tests]
- if: always()

Otherwise, the draft PR check won't work as intended.

[claude]

Code Review - PR #468: Ignore e2e-status on draft PR

Summary

This PR modifies the E2E workflow to skip the e2e-status job when a PR is in draft mode. The change is minimal (1 line modified) and follows the existing pattern used throughout the workflow.

Code Quality and Best Practices ✅

Positive:

• The change is consistent with the existing pattern: all other jobs (build, e2e-branch, and e2e-tests) already skip draft PRs using if: github.event.pull_request.draft == false (.github/workflows/e2e.yml:22, :64, :71)
• The condition placement is logical: if should come before other job properties for clarity
• Reordering from line 212 to line 210 improves readability by placing the condition at the top of the job definition

Observation:

• The pattern is repeated across 4 jobs. While this works, consider whether a workflow-level condition might be cleaner for future maintenance. However, GitHub Actions doesn't support top-level job conditions, so the current approach is acceptable.

Potential Bugs or Issues ✅

No issues found:

• The always() condition is preserved, ensuring this status job runs regardless of whether e2e-tests succeeds or fails
• The combined condition always() && github.event.pull_request.draft == false correctly ensures:
  • The job runs even if e2e-tests fails (via always())
  • But only for non-draft PRs (via github.event.pull_request.draft == false)
• The dependency on needs: [e2e-tests] remains intact

Performance Considerations ✅

Improvement:

• This change saves CI resources by not running the status check for draft PRs
• Consistent with the pattern where draft PRs skip all E2E-related jobs (build, e2e-branch, e2e-tests)
• Prevents unnecessary workflow runs and GitHub check status updates for draft PRs

Security Concerns ✅

No security issues:

• No sensitive data or security-related code is modified
• The change is purely workflow orchestration logic
• Draft PRs will still require passing E2E tests before merging (when they become ready for review)

Test Coverage

Testing notes:

• This is a GitHub Actions workflow change that can be verified by:
  1. Creating a draft PR and confirming e2e-status is skipped
  2. Converting to ready-for-review and confirming e2e-status runs
• Consider adding to the PR description: "Tested by verifying this draft PR skips all E2E jobs including e2e-status"

Additional Observations

Context: Looking at ci.yml:20, the same draft PR filtering pattern is used in the CI workflow, confirming this is a repository-wide convention.

Edge case handled: The needs: [e2e-tests] dependency is still respected, meaning if e2e-tests is skipped (because the PR is draft), then e2e-status will also be skipped, which is the desired behavior.

---

Verdict: LGTM ✅

This is a straightforward, well-implemented fix that:

• Completes the draft PR filtering pattern across all E2E workflow jobs
• Follows existing repository conventions
• Improves CI efficiency
• Has no negative side effects

Recommendation: Approve and merge.