[Feature] Adding CE installer - #313
Open
royischoss wants to merge 17 commits into
Open
Conversation
Move the MLRun CE installer from its standalone repo into this one, so it ships alongside the chart it installs. The installer keeps installing the PUBLISHED chart by default. This repo's chart is used only when the caller passes --chart-path ./charts/mlrun-ce. The script is also served via curl | bash, where no repo exists around it, so the same invocation has to mean the same thing in both places. Repo integration: - make installer-test / installer-lint targets (bats, bash -n, shellcheck) - Installer CI workflow: lint and unit tests on PRs touching scripts/**, plus a workflow_dispatch-only kind end-to-end install - gitignore ce-config.yaml so filled-in registry details can't be committed - installer linked from the root and chart READMEs Align the pre-install version validators with the chart's own prerequisites rather than the product install docs: Helm >= 3.6 blocking, matching charts/mlrun-ce/README.md, and no Kubernetes floor at all, since the chart declares no kubeVersion and the README states no cluster version. The previous K8s >= 1.34 / Helm >= 4.1 floors rejected nearly every supported cluster and every Helm 3 user for a chart that renders fine on Helm 3. MIN_K8S_VERSION now only warns; MIN_HELM_VERSION remains the one hard floor and can be raised to tighten.
workflow_dispatch-only kind end-to-end install
The resolve_external_host KUBE_CONTEXT test stubbed `command` but matched on the subcommand immediately after shifting off "kubectl", so it never saw the --context <ctx> that the kubectl wrapper injects ahead of the real arguments. The stub returned nothing, node_ip came back empty and the suggested host fell back to localhost. It passed locally anyway: bats aborts a test on the first failed assertion via set -e, and under macOS's system bash 3.2 that only holds for the last statement in a @test. The broken assertion was second-to-last, so it was swallowed and the test reported ok. CI runs bash 5, where it fails. Both AGENTS.md and the run-tests skill now warn about this, since any test whose stub drifts from the code can hide the same way. Also bump the chart to 0.12.0-rc.12: development is already at rc.11, so ct lint saw no version bump and failed. Co-authored-by: Cursor <cursoragent@cursor.com>
"load_config exits 1 when yq is not installed" hid yq by setting PATH=/usr/bin:/bin. That works on macOS, where yq lives in /opt/homebrew/bin, but the GitHub runners ship yq in /usr/bin — so on CI yq stayed on PATH, load_config parsed the file and returned 0, and the test failed. install.sh's yq guard was correct all along; only the test's isolation was wrong. Replace the hardcoded PATH with an _empty_bin helper pointing at a directory that provably holds no executables, so the assertion no longer depends on where the host installs yq. The neighbouring "no yq required when CONFIG_FILE is empty" test used the same hardcoded PATH and was therefore vacuous on CI; it now uses the helper too. Co-authored-by: Cursor <cursoragent@cursor.com>
It's a working design document rather than reference material for the chart, so it stays on disk and out of the PR via .git/info/exclude instead of the tracked .gitignore. Strip the four AGENTS.md pointers and the install.sh NodePort comment that referenced it, so nothing in the repo links to a file reviewers won't have. The NodePort note now points at REQUIRED_NODEPORTS, which is the actual source of truth for that list. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop the Phase 1-6 status list and the Phase 3 pre-work notes: they record how the installer was built rather than how it works, which belongs in the local design doc, not the repo. Two genuinely reusable pieces are kept and restated without the phase framing — the value precedence rule (flag > env > config > --values > chart defaults, and why --config and -f compose) now sits under the install flow it describes, and the version floors read as current policy instead of a "realigned from X" note. De-specify the bug entries: they cited a named internal lab cluster, and one paragraph pinned live state on it, including an SSH command with an internal hostname and node IP. The findings hold for any remote cluster reached via --kube-context, so they now say that instead, and the teardown command is given with a placeholder context. Co-authored-by: Cursor <cursoragent@cursor.com>
The test hardcoded a named internal lab cluster and its node IP. Neither means anything to a reader of this repo, and the address is a real private one. Swap in a placeholder context name and 192.0.2.10, from RFC 5737's documentation range, so the fixture is self-evidently fake. Co-authored-by: Cursor <cursoragent@cursor.com>
The installer had no version at all, so a user couldn't say which script they ran and a run couldn't be reproduced. Add --version/-v, reading the version from charts/mlrun-ce/Chart.yaml beside the script rather than storing a copy: bumping the chart bumps the installer, with nothing to carry forward by hand. Run standalone (curl | bash, or copied to a bin directory) there's no chart to read and nothing recording where the script came from, so it reports unknown instead of inventing a number. Tying the version to the chart rather than giving the installer its own is deliberate. The script encodes chart internals — the fixed NodePort list, and the --set value paths it writes — so an installer and a chart from the same tag are the only pairing guaranteed to agree, and a renamed value path would otherwise fail silently as a --set that does nothing. The documented curl URLs pointed at .../development/scripts/install.sh, which is a 404 today (scripts/ isn't on development yet) and would be a moving target once it isn't. Use a pinned mlrun-ce-<version> tag as the primary form, since chart-releaser already tags every release and those tags contain this script; keep development documented as the rolling alternative. No new release workflow is needed as a result. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🟡 Changes recommended
There are a few confirmed functional issues (GitHub Actions branch filter glob, CoreDNS patch namespace lookup, and --enable-otel collector not disabling auto-instrumentation toggles) that should be fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds a first-class MLRun CE installer (scripts/install.sh) into the CE chart repository, alongside docs and a dedicated CI workflow, while keeping chart behavior unchanged (published chart is still the default install target unless --chart-path is used).
Changes:
- Add
scripts/install.sh(Bash installer) with config-file support (ce-config.yaml), preflight validators, and install/uninstall flows. - Add installer documentation (
scripts/README.md,scripts/docs/*), example config, and internal conventions (scripts/AGENTS.md). - Add an installer unit test suite (
tests/install_tests.bats), Makefile targets, and a new GitHub Actions workflow (Installer CI); bump chart version to0.12.0-rc.12and link the installer from existing READMEs.
File summaries
| File | Description |
|---|---|
scripts/install.sh |
New single-file installer wrapping Helm install/upgrade, config loading, validators, and helpers (progress UI, notes parsing). |
tests/install_tests.bats |
New bats-core unit test suite for installer behavior (parsing/config/validators). |
scripts/README.md |
Installer top-level usage and flows (published chart vs local chart). |
scripts/docs/{parameters,configuration,faq}.md |
Detailed reference docs for flags/env/config schema, precedence, and gotchas. |
scripts/ce-config.yaml.example |
Example installer: config block for repeatable installs. |
.github/workflows/installer-ci.yaml |
New CI workflow for shellcheck + bats; optional kind E2E via workflow_dispatch. |
Makefile |
Add installer-test and installer-lint targets. |
charts/mlrun-ce/Chart.yaml |
Bump chart version to 0.12.0-rc.12. |
README.md, charts/mlrun-ce/README.md, CONTRIBUTING.md, AGENTS.md |
Link/document the installer and related tooling expectations. |
.gitignore |
Ignore filled-in ce-config.yaml. |
.claude/skills/* |
Update internal skills/docs to reflect installer/testing/versioning conventions. |
Review details
Suppressed comments (1)
scripts/install.sh:270
patch_coredns_for_registrylooks up theingress-nginx-controllerService in${NAMESPACE}, but ingress controllers are typically deployed iningress-nginx(or another dedicated namespace). This makes the CoreDNS patch silently skip in the common case.
patch_coredns_for_registry() {
local registry_host="$1"
local ingress_clusterip
ingress_clusterip="$(kubectl get svc ingress-nginx-controller \
--namespace "${NAMESPACE}" \
-o jsonpath='{.spec.clusterIP}' 2>/dev/null || true)"
- Files reviewed: 17/18 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+10
to
+14
| pull_request: | ||
| branches: | ||
| - development | ||
| - "[0-9]+.[0-9]+.x" | ||
| workflow_dispatch: |
Collaborator
Author
There was a problem hiding this comment.
Checked this against the docs and I don't think it holds — GitHub's branch filters aren't plain globs. The filter pattern cheat sheet defines + as "matches one or more of the preceding character" and [] as "matches one alphanumeric character listed in the brackets or included in ranges", so [0-9]+.[0-9]+.x does match 0.12.x.
It's also the same filter ci.yaml already uses, so changing it here would make the two workflows disagree about which branches they run on. Leaving as is.
Comment on lines
+235
to
+240
| # Docker installed | ||
| if ! command -v docker &> /dev/null; then | ||
| log_error "Docker is not installed or not in PATH." | ||
| log_info "Install Docker: https://docs.docker.com/get-docker/" | ||
| exit 1 | ||
| fi |
Collaborator
Author
There was a problem hiding this comment.
Leaving this one, deliberately rather than by omission.
The helm/kubectl checks use type -P for a reason specific to them: the script defines its own kubectl and helm wrapper functions (to inject --context), so command -v would always find the script's own wrapper and the check would never fail. There is no docker wrapper, so that reasoning doesn't carry over.
Going the other way has a real cost. type -P docker would reject setups where docker is legitimately an exported shell function — a podman shim, or a wrapper pointing at a remote host — even though every docker call the installer makes would work fine. And the docker info check on the next line already catches anything genuinely broken, with a better error message than a presence check could give.
…sion
The modes were encoded as flags — --uninstall, and install as the unnamed
default — which reads as a script rather than a CLI. Add a verb in front:
install, uninstall, version, help, dispatched by parse_command so parse_args
stays a pure flag parser.
Nothing that worked before stops working. A leading flag, or no arguments at
all, still means install, so every documented invocation and the curl | bash
one-liner are unchanged, and `uninstall` and --uninstall are the same thing.
A bare word that isn't a known command is an error rather than an install:
`mlrun-ce-installer unistall` should not deploy a cluster on a typo.
Two details worth knowing. main() expands COMMAND_ARGS through the
${a[@]+"${a[@]}"} guard because bash < 4.4 — including the macOS system bash
this is developed on — treats an empty array as unset under set -u, so an
argument-less run would otherwise abort; there's a test pinning that case.
And the uninstall assignment is a full if rather than [[ ]] && x=y, which
would return 1 and take errexit with it whenever the command wasn't uninstall.
Also suppress color when stdout isn't a terminal or NO_COLOR is set. The log
helpers previously emitted escape bytes unconditionally, which landed in CI
logs and any redirected output.
The installed command is now mlrun-ce-installer rather than mlrun-install.
Co-authored-by: Cursor <cursoragent@cursor.com>
deploy_local_registry() had no DRY_RUN guard, so it ran kubectl apply unconditionally. On a cluster without the namespace the apply failed and errexit aborted the run, making --local-registry --dry-run unusable. On a cluster where the namespace existed the apply succeeded, so a run advertised as rendering-only really deployed a registry Deployment and Service and reported success. The guard returns early under DRY_RUN, placed after the LOCAL_REGISTRY_URL assignment so the URL still reaches the rendered --set flags. Verified live: the dry run renders it into nuclio's registry_url ConfigMap and mlrun's api chief/worker deployments while creating nothing. CI missed this because the kind-install job uses --local-registry for a real install, never with --dry-run. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed functional/documentation issues in the installer around dry-run behavior, CoreDNS patch ingress service lookup, and password-file handling that can cause unintended cluster mutations or auth failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
Previously missed (4) — in code that hasn't changed since the last review.
scripts/install.sh:304
patch_coredns_for_registrylooks upingress-nginx-controllerin the MLRun namespace ($NAMESPACE), but ingress controllers are typically deployed in their own namespace (e.g.ingress-nginx). This makes the CoreDNS patch skip even when the controller exists, reducing the usefulness of the--local-registry --enable-ingressflow.
scripts/install.sh:452- Using
echoto return an env-var value can mis-handle values that look like echo flags (e.g.-n) and is less reliable for returning raw strings; preferprintffor output from helpers.
This issue also appears on line 470 of the same file.
scripts/install.sh:633
- Reading
REGISTRY_PASSWORD_FILEvia command substitution preserves trailing newlines (common at EOF), which can make the resulting docker-registry secret invalid and cause auth failures. Strip CR/LF when loading the password file.
scripts/install.sh:1505 --dry-runis documented as not creating resources, but--local-registrycurrently still deploys a Deployment/Service (and may patch CoreDNS). Dry-run should skip local-registry deployment to avoid mutating the cluster.
scripts/install.sh:470
prompt_or_envreturns the resolved value viaecho, which can behave unexpectedly for certain values (e.g. leading-n, backslashes). Useprintfto return the exact string.
echo "${value:-$default}"
- Files reviewed: 17/18 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Description
Adds the MLRun CE installer to this repo as
scripts/install.sh— a single-file bash wrapper aroundhelm install mlrun-ce/mlrun-cewith an optionalce-config.yamlconfig layer, pre-install validators, and a bats test suite. It ships alongside the chart it installs.Chart behaviour is unchanged. The installer installs the published chart by default; this repo's chart is used only when the caller passes
--chart-path ./charts/mlrun-ce. That is deliberate — the script is also served viacurl | bash, where no repo exists around it, so the same invocation means the same thing in both places.Implements CEML-731 (parent feature), CEML-732 (integrate the installer into the CE repo) and CEML-733 (installer CLI: pre-flight checks,
ce-config.yamlschema, install flow).🛠️ Changes Made
scripts/install.sh,scripts/ce-config.yaml.example, docs underscripts/docs/, andscripts/AGENTS.md(installer conventions and bug log)tests/install_tests.bats— 98 unit tests, no cluster requiredmake installer-test/make installer-lint, and anInstaller CIworkflow running shellcheck + bats on every PR, plus aworkflow_dispatch-only kind end-to-end installHELM_TIMEOUT(default960s) —--waitpreviously inherited helm's 5m default and marked the release failed mid image pull.gitignoreforce-config.yaml; installer linked from the root and chart READMEs; chart bumped to0.12.0-rc.11✅ Checklist
charts/mlrun-ce/Chart.yaml.No chart values changed, so the three install-mode values files need no update.
🧪 Testing
make installer-test), plusbash -nand shellcheck (make installer-lint) — all green--chart-path ./charts/mlrun-ceand with no chart flag (confirming the published chart is still the default)--kube-contextthrough an SSH tunnel: 33/33 containers ready,helm status→deployedDocker Desktop was exercised via dry-run only, so the "both environments" checklist item is left unchecked:
mlrun-uihas nolinux/arm64image and cannot come up on Apple Silicon.🔗 References
🚨 Breaking Changes?
🔍️ Additional Notes