Repository navigation
ci: use matrix object (path + name) for module matrix - #170
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Replace the bare matrix string entries with objects that carry both a
Go module path (used for 'go test', coverage files, cache key) and an
artifact-safe name (used in the coverage-artifact upload). This removes
the need for any string replacement / pipe filter in expressions, and
makes adding a new module a single - { path, name } line.
There was a problem hiding this comment.
🟡 Changes recommended
Correct the CI matrix path from pkg/argo/rollouts to pkg/argo/roll.
1 open finding
What changed in this PR
Refactors CI module matrices to use {path, name} objects and expands REST config tests.
Changes:
- Replaces pipe-based matrix expressions.
- Adds coverage artifact generation.
- Adds kubeconfig success and error-path tests.
- CI references nonexistent
pkg/argo/rolloutsinstead ofpkg/argo/roll.
| File | Summary |
|---|---|
.github/workflows/ci.yaml |
Updates module matrices and coverage handling; contains an incorrect module path. |
pkg/utils/restconfig_test.go |
Adds REST config loading and invalid-path tests. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - { path: pkg/velero, name: pkg-velero } | ||
| - { path: pkg/argo/cd, name: pkg-argo-cd } | ||
| - { path: pkg/argo/events, name: pkg-argo-events } | ||
| - { path: pkg/argo/rollouts, name: pkg-argo-rollouts } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf88f799dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - { path: pkg/velero, name: pkg-velero } | ||
| - { path: pkg/argo/cd, name: pkg-argo-cd } | ||
| - { path: pkg/argo/events, name: pkg-argo-events } | ||
| - { path: pkg/argo/rollouts, name: pkg-argo-rollouts } |
There was a problem hiding this comment.
Use pkg/argo/roll here, as registered in go.work:15 and .github/modules.json:12; there is no pkg/argo/rollouts directory. I checked the exact fmt command, and gofmt -l pkg/argo/rollouts exits with status 2 and no such file or directory. Because the same nonexistent path appears in all five matrices, the prerequisite fmt/vet/lint jobs fail, the dependent test job is skipped, and the actual rollout module is no longer checked.
Useful? React with 👍 / 👎.
bf88f79 to
7abdc7e
Compare
The Argo Rollouts module is the project's full name; the short 'roll' was always a placeholder. Rename the directory, update the Go module path in go.mod, and update go.work to point at the new location. Also updates NewArgoRolloutsClient to the variable-indirection pattern (var = func) so tests can inject a fake clientset. This matches the pattern used in pkg/argo/events and pkg/argo/workflows and is a prerequisite for the rollout test file. The CI matrix on ci/coverage-baseline already references 'pkg/argo/rollouts'; this commit makes the module path actually exist on disk so the matrix path resolves.
| - { path: pkg/velero, name: pkg-velero } | ||
| - { path: pkg/argo/cd, name: pkg-argo-cd } | ||
| - { path: pkg/argo/events, name: pkg-argo-events } | ||
| - { path: pkg/argo/rollouts, name: pkg-argo-rollouts } |
Problem
The last CI run on
main(workflowci.yaml) fails before any step runs. GitHub's workflow parser rejects the file:The workflow runs die in a single millisecond (
created_at == updated_at, 0 jobs, no logs) because the workflow file fails to load — GitHub's expression parser does not allow|(pipe) onmatrix.*references inside${{ }}.Fix
The CI matrix now uses matrix objects instead of bare strings. Each entry is
{ path, name }:gotooling (working-directory,go vet,go test, etc.) referencesmatrix.module.path.matrix.module.name(already hyphen-safe — module names use-not/).${{ matrix.module | replace(...) }}expressions remain in the workflow.This sidesteps the parser restriction entirely (no pipes inside
${{…}}) while keeping the matrix shape clean.Verification
Validated locally before pushing:
go vet ./...andgo test ./...againstpkg/utilswith the newworking-directoryvalue to confirm the per-module commands still work end-to-end.Tracking the CI run on this PR to confirm the green build on the merged result.