/test

Thanks for the fix. @fgiloux let's make sure to figure out a way we can test this properly before we reintroduce this change. I think typically changes to CI itself requires a reviewer to validate the branch works by pushing a copy to the cilium/cilium repository and running the tests from there (for example in a new dedicated PR). Not sure if that's the reason why this was not detected prior to merge, but it might be a hint. The background context to that is that modifying the tests we use to validate Cilium functionality prior to merge is a sensitive operation. (One of these days we should put together a linter that highlights this more clearly to contributors & reviewers so we can avoid such issues)

let's make sure to figure out a way we can test this properly before we reintroduce this change.

I don't have permissions to run tests. I don't have permissions to merge anything.
In this comment I hinted that it was not practical to test the change locally. From what I have seen this is not specific to this workflow but spread over Cilium CI estate, a lot of logic directly in GitHub workflows, which cannot be tested locally.

Looking at this run. It seems that it caught real issues.

@fgiloux I understand. Probably the best bet is to partner with a reviewer who can clone the branch into the cilium/cilium repo on your behalf, then work with them to validate the change.

Otherwise in this case I think the process is working more or less as intended: @giorio94 merged the PR, so took responsibility for potential breakages, which leads to this PR. We can't catch everything of course, but a little bit more coordination with reviewers before merge (particularly for CI changes) can help to minimize disruption to the tree. Perhaps it makes sense also to consider which areas of the codebase you might be able to help with review and look at moving you up the community ladder, then you could validate the changes in the Cilium tree independently?

Not sure if that's the reason why this was not detected prior to merge,

For security reasons, workflows are usually run from the main branch, not the PR branch, hence the runs on my PR probably did not have the change.

Perhaps it makes sense also to consider which areas of the codebase you might be able to help with review and look at moving you up the community ladder, then you could validate the changes in the Cilium tree independently?

Let's not make it a personal thing. I don't think this would resolve what I see as an issue with maintaining processes.

a little bit more coordination with reviewers before merge.

Here is how I see things: reviewers merged the PR, in good faith that it was working probably induced by the passing checks, which provided a misleading signal. One possible option to avoid this to happen again is to have a check, which requires a specific label for CI changes. When this check fails it provides instructions to the reviewers on how to push the changes to a dedicated branch with a workflow tweak so that it runs the new version and set the label when successful. It may even be possible to automate this step with Ariane.

Perhaps it makes sense also to consider which areas of the codebase you might be able to help with review and look at moving you up the community ladder, then you could validate the changes in the Cilium tree independently?

Let's not make it a personal thing. I don't think this would resolve what I see as an issue with maintaining processes.

My comment was made in good faith as a suggestion how to improve the process to minimize the impact to the community of 100+ developers working on Cilium on a regular basis. If you are interested in becoming a reviewer, that is one potential way to improve the overall bandwidth in the community (in addition to being a workaround for the issue we're discussing here).

a little bit more coordination with reviewers before merge.

Here is how I see things: reviewers merged the PR, in good faith that it was working probably induced by the passing checks, which provided a misleading signal. One possible option to avoid this to happen again is to have a check, which requires a specific label for CI changes. When this check fails it provides instructions to the reviewers on how to push the changes to a dedicated branch with a workflow tweak so that it runs the new version and set the label when successful. It may even be possible to automate this step with Ariane.

Yes I think this is a better solution to the underlying problem. So far we have relied on reviewers & committers "knowing" when a change might have this impact, but we have also broken the tree several times in the past few months based on the same pattern. It's probably the right time to make this process more clear by improving the tooling, so the expectations are more obvious for those reviewers. cc @cilium/ci-structure