Skip to content

fix(e2e): fix local minikube scripts and switch CLI health checks to /status - #393

Merged
matthewgrossman merged 8 commits into
mainfrom
mgrossman/aircore-786-local-docker-image-build-story-is-broken-for-nemo-platform
Jun 23, 2026
Merged

fix(e2e): fix local minikube scripts and switch CLI health checks to /status#393
matthewgrossman merged 8 commits into
mainfrom
mgrossman/aircore-786-local-docker-image-build-story-is-broken-for-nemo-platform

Conversation

@matthewgrossman

@matthewgrossman matthewgrossman commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Two fixes:

1. CLI and e2e health checks: /health/ready/status

The nemo CLI (services start, setup, quickstart) and e2e test harness were polling /health/ready to check if the platform is up. This endpoint is not exposed through ingress — by design, /health is for K8s pod probes, not external access. Switched to /status, which is already in the ingress allowlist and returns richer application-level health info.

Changed files: CLI source (packages/nemo_platform_ext/.../quickstart/cli.py, services/cli.py, setup.py), vendored SDK copies, e2e/conftest.py, e2e/k8s/scripts/run_auth_e2e.sh

2. Fix local_build_and_upgrade.sh

The script was broken in several ways:

  • Referenced nonexistent e2e/k8s/values/local.yaml
  • Didn't set api.image.repository or pullPolicy — pods tried to pull from nvcr.io instead of using local images
  • Used the GPU setup script (no port mapping) — localhost:30080 didn't work
  • Duplicated Helm install logic from install_helm_e2e.sh

Now delegates to install_helm_e2e.sh for the Helm install, uses the CPU setup script with port mapping, and supports configurable registry via NMP_REGISTRY env var. Also added NMP_E2E_PULL_POLICY to install_helm_e2e.sh so local builds can set pullPolicy=Never.

Test plan

  • minikube deletebash e2e/k8s/scripts/local_build_and_upgrade.sh succeeds end-to-end
  • curl localhost:30080/status returns 200
  • Full e2e suite against minikube: 54 passed, 2 failed (pre-existing), 8 skipped

Closes AIRCORE-786
Closes AIRCORE-791

🤖 Generated with Claude Code

Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
@matthewgrossman
matthewgrossman requested review from a team as code owners June 22, 2026 21:06
@github-actions github-actions Bot added the fix label Jun 22, 2026
@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds NMP_E2E_PULL_POLICY support to install_helm_e2e.sh for overriding image pull policies. Rewrites local_build_and_upgrade.sh to use profile-scoped minikube startup, configurable registry, and delegates Helm install to install_helm_e2e.sh via exec. Replaces all /health/ready readiness probe endpoints with /status across CLI commands and E2E scripts.

Changes

Helm Pull Policy Override and Local Build Delegation

Layer / File(s) Summary
NMP_E2E_PULL_POLICY in install_helm_e2e.sh
e2e/k8s/scripts/install_helm_e2e.sh
Declares NMP_E2E_PULL_POLICY variable (empty default) and conditionally appends --set api.image.pullPolicy and --set core.image.pullPolicy to HELM_ARGS.
Profile-aware build and exec delegation
e2e/k8s/scripts/local_build_and_upgrade.sh
Adds MINIKUBE_PROFILE-scoped startup via setup_local_minikube_cpu.sh, epoch-based IMAGE_TAG, NMP_REGISTRY-driven docker buildx bake docker-cpu, and replaces the direct helm upgrade --install call with exec install_helm_e2e.sh passing NMP_E2E_PULL_POLICY=Never.

Readiness Probe Endpoint Change to /status

Layer / File(s) Summary
/health/ready → /status everywhere
e2e/conftest.py, e2e/k8s/scripts/run_auth_e2e.sh, packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/quickstart/cli.py, .../services/cli.py, .../setup.py
All readiness poll targets switch from /health/ready to /status, with matching docstring/error-message updates.

Suggested labels

ci

Suggested reviewers

  • svvarom
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: fixing local minikube scripts and switching health checks from /health/ready to /status.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch mgrossman/aircore-786-local-docker-image-build-story-is-broken-for-nemo-platform

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

@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor
Suite Lines Covered Line Rate Branch Rate
Unit Tests 21176/27762 76.3% 61.2%
Integration Tests 12216/26531 46.0% 19.5%

@mckornfield mckornfield 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.

some minor things

Comment thread e2e/k8s/scripts/install_nmp_auth_e2e.sh Outdated
Comment thread e2e/k8s/scripts/local_build_and_upgrade.sh
Comment thread e2e/k8s/values/minikube.yaml Outdated
Comment thread e2e/k8s/values/minikube.yaml Outdated
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>

@mckornfield mckornfield 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.

some minor things

Comment thread k8s/helm/values.yaml Outdated
Comment thread e2e/k8s/values/minikube-auth-portforward.yaml Outdated
Comment thread e2e/k8s/values/minikube-auth-portforward.yaml
Comment thread e2e/k8s/values/minikube-auth.yaml
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>

@crookedstorm crookedstorm 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.

Looks good to me, for what it's worth.

Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
…ld-story-is-broken-for-nemo-platform

Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
@matthewgrossman matthewgrossman changed the title fix(e2e): Update scripts for minikube fix(e2e): fix local minikube scripts and switch CLI health checks to /status Jun 23, 2026
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

@matthewgrossman
matthewgrossman added this pull request to the merge queue Jun 23, 2026
Merged via the queue into main with commit 8111245 Jun 23, 2026
53 checks passed
@matthewgrossman
matthewgrossman deleted the mgrossman/aircore-786-local-docker-image-build-story-is-broken-for-nemo-platform branch June 23, 2026 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants