Skip to content

Configure required_status_checks.contexts in Prow branch protection (velero + plugin forks) #552

Description

@kaovilai

Summary

None of our OpenShift/OADP forks of Velero (or its plugins) currently configure required_status_checks.contexts in their Prow branch-protection config. This means merges are gated only by tide's label requirements (approved, lgtm, no hold/work-in-progress/etc.), not by any specific CI job actually passing. A PR can theoretically merge with failing GitHub Actions checks as long as the label conditions are met.

Prow's branch-protection Policy supports a required_status_checks field (ContextPolicy.Contexts []string — "Contexts appends required contexts that must be green to merge"):
https://github.com/kubernetes-sigs/prow/blob/0633879af8026d056e1a5dbe1e29f5a98f6acec3/pkg/config/branch_protection.go#L38

Our current config for openshift/velero has no such block:
https://github.com/openshift/release/blob/main/core-services/prow/02_config/openshift/velero/_prowconfig.yaml#L1

branch-protection:
  orgs:
    openshift:
      repos:
        velero:
          allow_force_pushes: true
          include:
          - oadp-dev
          - ^oadp-
          protect: true

Proposal

Add a required_status_checks.contexts list to the openshift/velero prowconfig (and the sibling plugin-fork prowconfigs — see below), e.g.:

branch-protection:
  orgs:
    openshift:
      repos:
        velero:
          allow_force_pushes: true
          include:
          - oadp-dev
          - ^oadp-
          protect: true
          required_status_checks:
            contexts:
            - ci/prow/lint
            - ci/prow/images
            - Run CI
            - Run Linter Check

Context names pulled from a recent openshift/velero PR (#494/#517) so the list reflects the actual status/check names GitHub reports today:

  • Prow-native contexts (from the commit status API): ci/prow/unit-test, ci/prow/images, ci/prow/lint
  • GitHub Actions checks currently running unguarded in the fork: Run CI, Run Linter Check, Check for invalid characters in file paths, Run Codespell, run-e2e-test (matrix), build

This lines up with the review discussion on velero-io#10001 (which added if: github.repository == 'velero-io/velero' guards to several workflows so they skip on forks). In that PR, @kaovilai specifically called out checks that should keep running — and by extension be good candidates for "required" — in the openshift/velero fork:

  • e2e-test-kind.yaml: "We wanted this in our fork, so we would like to keep running these tests in the forks if possible. There is nothing inherently specific to the upstream repo here, as evidenced by the many passing rates in our forks of these jobs."
  • pr-linter-check.yml: "I think we still want these so that if we develop features that will be later put back on upstream, the lint is up to date the config in upstream."
  • nightly-trivy-scan.yml: "We wanted this in our downstream fork."
  • pr-filepath-check.yml: "Though we probably also want this" — note this one was guarded by that PR (now skipped on forks) despite that comment, which is worth double-checking/reverting if we want it back as a required check here.

Reduce duplicate test effort: Makefile.prow vs. upstream GitHub Actions

Makefile.prow (in openshift/velero) is what backs the Prow-native ci/prow/unit-test and ci/prow/lint contexts:

https://github.com/openshift/velero/blob/main/Makefile.prow

.PHONY: ci
ci: verify-modules verify all test

.PHONY: test
test: setup-env
	...
	KUBEBUILDER_ASSETS=$(KUBEBUILDER_ASSETS) GOFLAGS=$(GOFLAGS) hack/test.sh

.PHONY: lint
lint: golangci-lint
	PATH=$(PROWBIN):$(PATH) GOFLAGS=$(GOFLAGS) hack/lint.sh

This wraps the same hack/test.sh / hack/lint.sh that upstream velero-io/velero runs via its own GitHub Actions (Run CImake ci, Run Linter Check). So today we run the equivalent test/lint twice on every PR: once through Prow (ci/prow/unit-test, ci/prow/lint, driven by Makefile.prow), and once through the GitHub Actions inherited from upstream (Run CI, Run Linter Check). That's duplicate CI effort, and it means test/lint fixes or config drift have to be reconciled in two separate places (Makefile.prow here vs. hack/*.sh and workflow files upstream) instead of one.

Proposal: retire the redundant Prow-native ci/prow/unit-test / ci/prow/lint jobs (or point Makefile.prow at the same invocation path, not a separate wrapper) and standardize on the upstream-aligned GitHub Actions checks (Run CI, Run Linter Check) as the source of truth for required status checks. That keeps us upstream-aligned on tests — we get upstream's test fixes for free, and any test fixes we make land in the same place upstream would look for them, instead of being siloed in Makefile.prow.

Plugin forks (aws / gcp / microsoft-azure)

Filing this against openshift/velero only, since the plugin fork repos (openshift/velero-plugin-for-aws, openshift/velero-plugin-for-gcp, openshift/velero-plugin-for-microsoft-azure) don't have an Issues tab enabled.

Each of these forks has an upstream in velero-io and has the same gap: their _prowconfig.yaml (e.g. core-services/prow/02_config/openshift/velero-plugin-for-aws/_prowconfig.yaml) also has no required_status_checks. Each currently runs a single unguarded GitHub Actions workflow (.github/workflows/pr.yaml, job named Run CI, doing make ci + Codecov upload) plus the shared Prow ci/prow/* contexts. The same fix — and the same Makefile.prow-vs-upstream-CI de-duplication question — applies there too:

required_status_checks:
  contexts:
  - Run CI

(openshift/velero-plugin-for-csi and openshift/velero-plugin-for-legacy-aws are excluded from this ask — they have no velero-io upstream.)

Ask

  • Add required_status_checks.contexts to the openshift/velero prowconfig in openshift/release, and the same to the three plugin-fork prowconfigs listed above.
  • Confirm which of the currently-unguarded checks should be in the required list (the ones cited above are a starting proposal based on PR Skip upstream-only workflows on forks velero-io/velero#10001's review comments, not final).
  • Double check whether pr-filepath-check.yml being skipped on forks (per Skip upstream-only workflows on forks velero-io/velero#10001) conflicts with wanting it as a required check.
  • Decide whether to retire the Makefile.prow-driven ci/prow/unit-test / ci/prow/lint Prow jobs in favor of the upstream-aligned GitHub Actions checks, to stop running duplicate test/lint effort and stay aligned with upstream fixes.

Note

Responses generated with Claude

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions