k8s-ci-robot merged approved PR with `needs-ok-to-test` label

[janetkuo]

What happened: This PR was merged after /lgtm and /approve: kubernetes-sigs/agent-sandbox#288, even though it hasn't passed tests (still with needs-ok-to-test label)

What you expected to happen: I'd expect the PR not merged until ok-to-test label exists and tests have passed

[janetkuo]

/sig testing

[Pnkcaht]

Solved @janetkuo :)

[cblecker]

@janetkuo Going to answer your question here:
ok-to-test only applies to tests Prow is aware of, and uses branch protection rules to prevent merges if things aren't passing. Looking at the tests on that PR, they appear to be just GitHub actions tests.

Options to address:

1. Convert all your tests into prow jobs, so that prow is fully aware of the jobs and can enforce proper branch protection
2. Manually make prow aware that you're using GitHub actions adding the additional required checks to the tide branch protection config to ensure that they need to pass before merging. For example:

   test-infra/config/prow/config.yaml

   Lines 622 to 628 in 9c058c9

   	kustomize:
   	required_status_checks:
   	contexts:
   	- Lint
   	- Test Linux
   	- Test MacOS
   	- Test Windows

Option 1 would take more work, but preserves the existing test workflows like the rest of the Kubernetes project and handles things automatically. Option 2 is easier, but you need to manually maintain that list to ensure that the tests you care about are respected.

[janetkuo]

@cblecker thanks for the detailed explanation! I didn't configure GitHub actions or any of those tests workflows in the agent-sandbox repo. They seem to come from GitHub Copilot code review, triggered by the PR author.

I suspect that technically anyone could bypass prow tests in other Kubernetes projects via Copilot code review.

[cblecker]

@janetkuo Oh, I see that now. What prow tests did you expect to run before this PR merged then?

[janetkuo]

@cblecker Here's the list of prow tests that run before a normal PR is merged:

e.g., kubernetes-sigs/agent-sandbox#309 (comment)

[janetkuo]

There's another PR in agent-sandbox repo that got merged with needs-ok-to-test label, and I didn't see prow jobs or GitHub workflow running in that PR, and it's not related to GitHub Copilot code review:
kubernetes-sigs/agent-sandbox#315 (comment)

[petr-muller]

@janetkuo looking at both kubernetes-sigs/agent-sandbox#315 and kubernetes-sigs/agent-sandbox#288 PRs, they only change .md files.

All four presubmits configured for the repo have either a skip_if_only_changed field:

skip_if_only_changed: "^docs/|\\.md$|^(\\.gitignore|LICENSE|netlify\\.toml|OWNERS)$"

or a run_if_changed one:

run_if_changed: "^.*\\.go$|^go\\.(mod|sum)$|^dev/ci/presubmits/lint-go$"

which in both cases make the job not trigger on PRs that only touch markdowns. An /ok-to-test command would just switch labels from the red one to the green one, but would launch no jobs, and Tide in turn can merge it with just the approval/lgtm labels, because it merges on "all required triggered by the PR jobs passed" criteria which in this case means none.

[janetkuo]

@petr-muller ah, that makes sense! So it's probably safe to ignore this issue.