Skip to content

chore(ncp-local-cluster): fix stale and missing .PHONY entries#309

Merged
kristinapathak merged 1 commit into
NVIDIA:mainfrom
mesutoezdil:mesutoezdil/chore/ncp-local-cluster-makefile-phony
Jul 21, 2026
Merged

chore(ncp-local-cluster): fix stale and missing .PHONY entries#309
kristinapathak merged 1 commit into
NVIDIA:mainfrom
mesutoezdil:mesutoezdil/chore/ncp-local-cluster-makefile-phony

Conversation

@mesutoezdil

@mesutoezdil mesutoezdil commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

TL;DR

.PHONY declared build-linux-amd64, which does not exist, and was missing seven real targets (ensure-cluster, setup-metallb, deploy-nginx, wait-for-nginx, wait-for-gateway, validate-gateway, cleanup-nginx). Declaration-only change, no recipes touched; make validate-compute-clusters passes and make -n parses all targets. A .PHONY contract test would touch the file #310 modifies, so it is left as a follow-up.

Issues

Closes #306

Drop build-linux-amd64 from .PHONY; the target does not exist. Add
the real targets ensure-cluster, setup-metallb, deploy-nginx,
wait-for-nginx, wait-for-gateway, validate-gateway, and
cleanup-nginx so a same-named file cannot silently skip them. Also
replace an em dash in a comment per the ASCII-only style rule.

Closes NVIDIA#306

Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
@mesutoezdil
mesutoezdil requested a review from a team as a code owner July 21, 2026 12:16
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The local cluster Makefile removes the obsolete build-linux-amd64 phony target, adds lifecycle and validation targets to .PHONY, and clarifies .env variable override precedence.

Changes

Local cluster Makefile

Layer / File(s) Summary
Phony targets and environment configuration
tools/ncp-local-cluster/Makefile
.PHONY now reflects supported lifecycle, Gateway, nginx, and cleanup targets, while the .env documentation clarifies that command-line variables override configuration rather than shell environment variables.

Estimated code review effort: 1 (Trivial) | ~2 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address the linked issue by removing the nonexistent target and marking the real cluster-related targets phony.
Out of Scope Changes check ✅ Passed The extra phony entries and comment tweak stay within the Makefile maintenance scope and do not introduce unrelated behavior changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: fixing stale and missing .PHONY entries in the ncp-local-cluster Makefile.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 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 `@tools/ncp-local-cluster/Makefile`:
- Line 16: Add focused regression coverage for the .PHONY target list in the
Makefile, verifying that creating same-named files does not suppress execution
of the relevant targets; place it in the existing test-multicluster-make.sh
coverage or an appropriate repository-native test. Ensure the test runs through
the listed validation commands and preserves the current help and
port-substitution checks.
- Around line 22-25: Update the .env precedence comments near the Makefile’s
environment-loading configuration to accurately state that .env assignments
override imported shell variables, while removing a key does not necessarily
unset it: the shell value may still apply through ?=, or the configured default
may be used. Keep the command-line override behavior documented.
🪄 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: Enterprise

Run ID: f993f341-641a-4889-b46e-a41a83677e9f

📥 Commits

Reviewing files that changed from the base of the PR and between 5ef95ed and fe7e18f.

📒 Files selected for processing (1)
  • tools/ncp-local-cluster/Makefile

Comment thread tools/ncp-local-cluster/Makefile
Comment thread tools/ncp-local-cluster/Makefile
@kristinapathak
kristinapathak requested a review from sbaum1994 July 21, 2026 17:20
@kristinapathak
kristinapathak added this pull request to the merge queue Jul 21, 2026
Merged via the queue into NVIDIA:main with commit 7f05db5 Jul 21, 2026
16 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ncp-local-cluster: stale and missing .PHONY entries in Makefile

3 participants