The comment above the checks job in .github/workflows/pull-requests.yaml says the unit and controller tests gate the pull request:
# Helm-unittest + Go controller tests. Decoupled from the image build and from
# e2e so they give fast, independent signal; they gate the PR as their own
# required check rather than blocking e2e from starting.
Branch protection on main says otherwise:
$ gh api repos/cozystack/cozystack/branches/main/protection --jq .required_status_checks.contexts
["pre-commit","E2E Tests"]
Unit & controller tests is not in that list, and the rules endpoint carries only a pull_request rule with no required_status_checks of its own. So a red unit or controller test is visible on the PR page and does not block the merge button.
The decoupling half of the comment is accurate and worth keeping: the job really is independent of the image build and of e2e, and that is why it gives fast signal. Only the claim about gating is wrong.
This is worth fixing rather than ignoring because the comment is where somebody goes to find out whether CI will catch them. I watched exactly that happen: a comment written elsewhere in the tree picked up the word "required" from here, and the mistake was only caught by querying the protection API directly.
Two ways to resolve it, and the choice is a decision about the repository rather than an editorial one:
- Correct the comment to say what is true today, that the job reports independently and does not gate the merge.
- Add
Unit & controller tests to the required contexts, which makes the comment true and closes the gap it describes. A red make unit-tests or make test-controllers currently reaches main unimpeded as long as e2e and pre-commit are green.
The second is the larger change and needs an owner's call, so the comment should not simply be edited to match the weaker state without someone deciding that the weaker state is what is wanted.
The second half: a red unit run hides the controller run
The same job runs two targets as consecutive steps, make unit-tests then make test-controllers. A failure in the first ends the job, so the second never executes and reports nothing at all. That is not specific to any one package: everything under internal/... is invisible whenever the helm-unittest half is red.
Together with the gating gap above this reads worse than either half alone. The job does not block the merge, and when it does fail it tells you about at most one of the two things it runs. Whoever picks this up gets both for the price of one: adding the job to the required contexts and letting the second step run regardless (if: always() or splitting the targets into separate jobs) are independent changes, and either is useful without the other.
The comment above the
checksjob in.github/workflows/pull-requests.yamlsays the unit and controller tests gate the pull request:Branch protection on
mainsays otherwise:Unit & controller testsis not in that list, and the rules endpoint carries only apull_requestrule with norequired_status_checksof its own. So a red unit or controller test is visible on the PR page and does not block the merge button.The decoupling half of the comment is accurate and worth keeping: the job really is independent of the image build and of e2e, and that is why it gives fast signal. Only the claim about gating is wrong.
This is worth fixing rather than ignoring because the comment is where somebody goes to find out whether CI will catch them. I watched exactly that happen: a comment written elsewhere in the tree picked up the word "required" from here, and the mistake was only caught by querying the protection API directly.
Two ways to resolve it, and the choice is a decision about the repository rather than an editorial one:
Unit & controller teststo the required contexts, which makes the comment true and closes the gap it describes. A redmake unit-testsormake test-controllerscurrently reachesmainunimpeded as long as e2e and pre-commit are green.The second is the larger change and needs an owner's call, so the comment should not simply be edited to match the weaker state without someone deciding that the weaker state is what is wanted.
The second half: a red unit run hides the controller run
The same job runs two targets as consecutive steps,
make unit-teststhenmake test-controllers. A failure in the first ends the job, so the second never executes and reports nothing at all. That is not specific to any one package: everything underinternal/...is invisible whenever the helm-unittest half is red.Together with the gating gap above this reads worse than either half alone. The job does not block the merge, and when it does fail it tells you about at most one of the two things it runs. Whoever picks this up gets both for the price of one: adding the job to the required contexts and letting the second step run regardless (
if: always()or splitting the targets into separate jobs) are independent changes, and either is useful without the other.