Skip to content

chore: teach Make how to vendor#16394

Merged
Joibel merged 3 commits into
argoproj:mainfrom
Joibel:make-vendor
Jul 8, 2026
Merged

chore: teach Make how to vendor#16394
Joibel merged 3 commits into
argoproj:mainfrom
Joibel:make-vendor

Conversation

@Joibel

@Joibel Joibel commented Jul 7, 2026

Copy link
Copy Markdown
Member

Motivation

Make blindy runs go mod vendor but not in all the right places any more, so you quite often have to do it manually.

Modifications

Teach make how to vendor correctly so it only happens when actually needed with a vendor/modules.txt rule.

Add that as a proper pre-req to places that did it manually and quite a few that should need it.

Verification

CI will prove a lot. Ran a bunch of make targets locally too.

Documentation

Not required, dev facing change

AI

Claude did a sweep for extra targets and wrote me the rule for protoc.

Summary by CodeRabbit

  • Build and Tooling
    • Improved build reliability by treating vendor/modules.txt as the freshness check for vendored dependencies.
    • Updated build, code generation, linting, tests, documentation, and feature generation workflows to refresh outputs based on an up-to-date vendored state.
    • Removed conditional vendoring during protobuf-driven swagger/openapi generation to keep generated artifacts consistent.
  • CI
    • Updated the e2e “Prepare binaries” step to run make vendor/modules.txt before marking dist/* executable and touching build outputs.

Signed-off-by: Alan Clucas <alan@clucas.org>
@Joibel Joibel requested a review from Copilot July 7, 2026 13:27
@Joibel Joibel requested a review from a team as a code owner July 7, 2026 13:27
@Joibel Joibel added the area/contributing Contributing docs, ownership, etc. Also devtools like devcontainer and Nix label Jul 7, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the build workflow to make vendoring deterministic and only run when needed by introducing vendor/modules.txt as the canonical dependency for targets that require a fresh vendor/ tree.

Changes:

  • Add a vendor/modules.txt target that runs go mod vendor and use it as a prerequisite across build, codegen, lint, test, and doc generation targets.
  • Ensure protoc-generated swagger targets depend on a fresh vendor tree (excluding the _.primary/_.secondary swagger files).
  • Adjust lint-go to ensure the vendor tree is consistent after go mod tidy (non-Nix paths).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread Makefile Outdated
@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ad1954e8-bb27-427d-9cdc-c7866b5ffd15

📥 Commits

Reviewing files that changed from the base of the PR and between 0226cce and e26d17f.

📒 Files selected for processing (1)
  • .github/workflows/ci-build.yaml

📝 Walkthrough

Walkthrough

This PR adds a vendor/modules.txt sentinel target in the Makefile and updates build, codegen, lint, test, docs, and CI preparation steps to depend on it. It also removes a redundant conditional vendor refresh from the protobuf generation path.

Changes

Vendor Sentinel Adoption

Layer / File(s) Summary
vendor/modules.txt target and swagger/protoc wiring
Makefile
Adds a new vendor/modules.txt rule that runs go mod vendor and touches the output; protoc-generated swagger outputs now depend on it, and the old conditional vendor-exists check is removed from the protoc macro.
Binary build target prerequisites
Makefile
dist/argo-windows-%, dist/argo-%, dist/workflow-controller, and dist/argoexec switch prerequisites from go.sum to vendor/modules.txt.
Codegen and protobuf generation prerequisites
Makefile
codegen, generated.proto, and zz_generated.deepcopy.go gain vendor/modules.txt as a prerequisite; redundant vendor-exists check removed from go-to-protobuf recipe.
Lint flow re-vendor handling
Makefile
lint-go depends on vendor/modules.txt (non-Nix); a post-go mod tidy check re-runs go mod vendor if go.mod/go.sum are newer than vendor/modules.txt.
Test and docs generation prerequisites
Makefile
test-%, Test%, Benchmark%, docs/metrics.md, docs/tracing.md, docs/variable-flow/variables.md, openapi_generated.go, and hack/featuregen/featuregen add vendor/modules.txt as a prerequisite.
CI prepare binaries vendor refresh
.github/workflows/ci-build.yaml
The e2e-tests job now runs make vendor/modules.txt before chmod +x dist/* and make --touch dist/*.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise, conventional, and clearly summarizes the main change to Make-based vendoring.
Description check ✅ Passed The description follows the template sections and explains the motivation, changes, verification, docs, and AI usage.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@Makefile`:
- Line 878: The swagger conditional in the Makefile uses an invalid GNU Make
directive, so the block cannot be parsed correctly. Update the conditional near
the swagger section from the misspelled directive to the proper GNU Make form,
ensuring the matching endif for that block remains valid and the rest of the
Makefile continues to parse.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a283a056-b2fb-4854-8276-936ca586fe49

📥 Commits

Reviewing files that changed from the base of the PR and between 4814bc4 and 656f189.

📒 Files selected for processing (1)
  • Makefile

Comment thread Makefile Outdated
Joibel and others added 2 commits July 7, 2026 14:55
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Alan Clucas <alan@clucas.org>
`make --touch` doesn't run recipes, so without a vendor tree present it
creates an empty vendor/modules.txt, which flips Go into vendor mode and
breaks every later go build in the job with "inconsistent vendoring".

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Alan Clucas <alan@clucas.org>
@Joibel Joibel merged commit d0ebcda into argoproj:main Jul 8, 2026
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/contributing Contributing docs, ownership, etc. Also devtools like devcontainer and Nix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants