fix: outdated and missing targets #305
Conversation
WalkthroughEnvironment setup switched from a fixed sourced env to dynamic variables ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer/Make
participant Env as .envrc (shell)
participant Make as Makefile targets
participant Kube as Kubernetes cluster actions
Dev->>Env: load env (.envrc)
note right of Env #DDEFEF: AP_RUN_NAME=$(basename "$PWD")\nAP_RUN_DIR="${DEVCACHE_RUN}/${AP_RUN_NAME}"\nAKASH_HOME="${AP_RUN_DIR}/.akash"
Env-->>Dev: exports AP_RUN_NAME, AP_RUN_DIR, AKASH_HOME
Dev->>Make: make kube-cluster-setup-gpu
Make->>Make: run $(KUBE_CREATE)
Make->>Make: kube-cluster-check-info
Make->>Make: kube-setup-ingress
Make->>Make: kube-prepare-images
Make->>Kube: kube-install-helm-charts
Make->>Kube: kube-setup-$(AP_RUN_NAME)
Make->>Kube: kube-deployments-rollout (phony)
Make->>Make: kube-setup-ssh (phony)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
_run/ssh/Makefile (1)
61-65: Consider adding implementation or documentation for empty targetsThe new phony targets
kube-deployments-rolloutandkube-setup-sshare currently empty. If these are intended as placeholders for future implementation, consider adding a comment to indicate this..PHONY: kube-deployments-rollout kube-deployments-rollout: + # TODO: Implement deployment rollout logic .PHONY: kube-setup-ssh kube-setup-ssh: + # TODO: Implement SSH setup logic
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
_run/ssh/.envrc(1 hunks)_run/ssh/Makefile(2 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
_run/ssh/Makefile
[warning] 64-64: Missing required phony target "all"
(minphony)
[warning] 64-64: Missing required phony target "clean"
(minphony)
[warning] 64-64: Missing required phony target "test"
(minphony)
🔇 Additional comments (5)
_run/ssh/.envrc (2)
8-8: LGTM: Dynamic AKASH_HOME configurationThe update to use
AP_RUN_DIRforAKASH_HOMEmakes the configuration more flexible and context-sensitive, which should help address the outdated targets issue mentioned in the PR objectives.
11-12: LGTM: Proper export of new variablesThe export of
AP_RUN_NAMEandAP_RUN_DIRensures these variables are available to the Makefile and other scripts that depend on them._run/ssh/Makefile (3)
28-28: LGTM: Explicit kubeconfig specificationAdding the explicit
--kubeconfig "$(AKASH_KUBECONFIG)"parameter ensures the provider uses the correct kubeconfig file, which aligns with the environment variable setup in.envrc.
50-53: LGTM: Enhanced cluster setup prerequisitesThe addition of
$(KUBE_CREATE),kube-cluster-check-info, andkube-setup-ingressas prerequisites provides a more comprehensive setup process for the GPU cluster.
58-59: LGTM: Dynamic target integrationThe use of
kube-setup-$(AP_RUN_NAME)makes the setup process context-aware and should help address the outdated targets issue mentioned in the PR objectives.
| export AKASH_KUBECONFIG=$KUBECONFIG | ||
| export AP_KUBECONFIG=$KUBECONFIG | ||
| export AP_RUN_NAME | ||
| export AP_RUN_DIR |
There was a problem hiding this comment.
export KUBE_SSH_NODES="root@<YOUR_CLUSTER_IP>" will still need to be manually added. CC @chainzero
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
_run/ssh/Makefile (3)
63-64: Generalize kube-setup-ssh to a pattern rule so other run names don’t break
- The empty
kube-setup-sshstub satisfies the currentAP_RUN_NAME=sshcase, but any future run-name will fail at parse time unless you add new stubs. Prefer a pattern rule.Apply this diff to future-proof:
-.PHONY: kube-setup-ssh -kube-setup-ssh: +.PHONY: kube-setup-% +kube-setup-%: + @echo "kube setup hook for '$*' has no additional steps; continuing"
60-64: Optional: Address checkmake minphony warnings (all/clean/test) locally or silence for run/ssh
- Static analysis flagged missing phony targets “all”, “clean”, “test”. Given this is an environment-specific Makefile, consider lightweight aliases to satisfy linters without altering behavior:
+.PHONY: all clean test +all: kube-cluster-setup-gpu +clean: clean-gpu +test: + @echo "No tests for SSH dev-env Makefile"
- Alternatively, if your project standard is to only define these at the top-level, consider excluding
_run/ssh/Makefilefrom that check in CI.
49-51: Update SSH Makefile: remove unnecessary fallback and optionally enforce sequential cluster setupAfter inspecting
_run/common-kube.mk, we see:
KUBE_CREATE := $(AP_RUN_DIR)/.kube-createis set unconditionally, so introducing a?=fallback in_run/ssh/Makefileis redundant.- To guarantee that the
kube-cluster-setup-gpuprerequisites run in order when users invokemake -j, you may optionally disable parallelism for this target.Locations to update:
- File:
_run/ssh/Makefile- Target:
kube-cluster-setup-gpuSuggested diff:
--- a/_run/ssh/Makefile +++ b/_run/ssh/Makefile @@ -.PHONY: kube-cluster-setup-gpu +kube-cluster-setup-gpu: init \ @@ kube-setup-ssh +# Prevent parallel execution to preserve prerequisite order under make -j +.NOTPARALLEL: kube-cluster-setup-gpuNo changes needed for
KUBE_CREATE.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
_run/ssh/Makefile(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
_run/ssh/Makefile
[warning] 63-63: Missing required phony target "all"
(minphony)
[warning] 63-63: Missing required phony target "clean"
(minphony)
[warning] 63-63: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: release-dry-run
- GitHub Check: integration-tests / crd-e2e
- GitHub Check: build-bins
🔇 Additional comments (1)
_run/ssh/Makefile (1)
60-62: No commonkube-deployments-rolloutto override—local stub is the sole implementation
I’ve checked the included files and there is nokube-deployments-rollouttarget in_run/common-kube.mk,_run/common-commands.mk, or_run/common.mk. Each cluster-specific Makefile (_run/ssh, _run/single, run/kube) provides its own rollout logic (or stub), so removing the SSH stub would break its rollout step rather than restore common behavior.Likely an incorrect or invalid review comment.
This PR fixes outdated and missing targets in the SSH development environment.
It will also help streamline the engineering documentation for SSH development environment.