test: add the Helm end-to-end integration workflow - #3595
Open
danish9039 wants to merge 3 commits into
Open
Conversation
A clone of full_kubeflow_integration_test.yaml that installs every component with a merged chart through helm install and keeps the identical test suite. Each install lives in tests/<component>_helm_install.sh, mirroring the Kustomize script it replaces, and uses the ci values file of the chart's comparison scenario, so the installed manifests are exactly the ones the comparison harness proves equivalent. Components without a chart keep their Kustomize scripts; a chart pull request adds its own install swap, the same ownership model as ci/comparison.yaml. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Member
Author
|
/retest |
1 similar comment
Member
Author
|
/retest |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
In one revision Helm pre-creates the namespaces its NetworkPolicies reference, and on a fast runner that bare namespace wins the race against the chart's labeled Namespace manifest. kubeflow-system then lacks istio-injection, the trainer pod starts without a sidecar and without the service.istio.io/canonical-name label, and the trainer-webhook NetworkPolicy no longer admits the API server's webhook calls. Install the Namespace objects first, add the policies by upgrade, and assert the labels landed. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
danish9039
marked this pull request as ready for review
September 1, 2026 16:48
Member
Author
|
@juliusvonkohout for review |
Member
|
To install is the verb, while the installation is the noun please keep this in mind and do not confuse the two in your text. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The workflow must run on pushes to master; the noted shell quoting and namespace-install rationale should also be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a Helm-based end-to-end integration workflow while retaining the existing platform test suite and Kustomize installs where Helm coverage is unavailable.
Changes:
- Adds Helm installation scripts for eight components.
- Adds a path-filtered Helm integration workflow.
- Reuses the existing end-to-end verification suite.
File summaries
| File | Description |
|---|---|
tests/oauth2-proxy_helm_install.sh |
Installs OAuth2 Proxy through Helm. |
tests/notebooks_helm_install.sh |
Installs Notebooks through Helm. |
tests/multi_tenancy_helm_install.sh |
Installs multi-tenancy roles through Helm. |
tests/kubeflow_namespace_helm_install.sh |
Installs Kubeflow namespaces and network policies. |
tests/katib_helm_install.sh |
Installs Katib through Helm. |
tests/dex_helm_install.sh |
Installs Dex through Helm. |
tests/dashboard_helm_install.sh |
Installs the Dashboard through Helm. |
tests/cert_manager_helm_install.sh |
Installs Certificate Manager through Helm. |
.github/workflows/helm_kubeflow_integration_test.yaml |
Defines the Helm integration workflow. |
Review details
Suppressed comments (3)
.github/workflows/helm_kubeflow_integration_test.yaml:14
- These wildcards also launch this resource-intensive workflow for charts that it does not install, including
common/istio/helmand the Hub and KServe UI charts underexperimental/helm/charts. A successful run therefore cannot validate those changed installation paths. Restrict the filters to the eight charts currently installed here, and let each future chart add its path together with its Helm installation step.
- common/*/helm/**
- applications/*/helm/**
- applications/*/*/helm/**
- experimental/helm/charts/**
tests/katib_helm_install.sh:12
- Quote this environment variable. The repository applies ShellCheck to changed test shell scripts, and this expansion triggers SC2086. Quoting also keeps the command argument stable.
kubectl label namespace $KF_PROFILE katib.kubeflow.org/metrics-collector-injection=enabled --overwrite
tests/kubeflow_namespace_helm_install.sh:11
- The stated race cannot be caused by this command: it does not pass
--create-namespace, Helm does not create namespaces merely because namespaced resources reference them, and Helm 4.2.2 ordersNamespacebeforeNetworkPolicyduring installation. This explanation therefore does not justify the additional installation revision and can hide the actual failure mechanism. Prefer one installation with the network policies enabled, or document a reproducible Helm defect that requires the two-phase workaround.
# Two phases: the first release revision renders only the Namespace objects,
# the second adds the NetworkPolicies. In one revision Helm pre-creates the
# namespaces its NetworkPolicies reference, and on a fast runner that bare
# namespace wins the race against the chart's own labeled Namespace manifest,
# leaving kubeflow-system without istio-injection and Pod Security labels.
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Member
|
Please also add the remaining components such as pipelines |
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.
What this does
Implements the mentor's direction for Helm integration testing: reuse the existing end-to-end workflow and replace the Kustomize install steps with Helm installs where charts exist.
helm_kubeflow_integration_test.yamlis a clone offull_kubeflow_integration_test.yamlwith only the install steps changed — the entire test suite (dex_login_test.py, the V1 and V2 pipeline runs, the unauthorized-token check, Katib, KServe, Spark, notebook execution, restricted Pod Security Standards) runs unchanged. The comparison harness already proves the rendered manifests are equivalent; this workflow proves the install path.Install steps swapped to Helm
tests/kubeflow_namespace_helm_install.shcommon/kubeflow-namespace/helmci/values-default.yamltests/multi_tenancy_helm_install.shcommon/kubeflow-roles/helmci/values-default.yamltests/cert_manager_helm_install.shcommon/cert-manager/helmci/values-base.yamlthenci/values-kubeflow.yaml, mirroring the base-then-overlay ordertests/dex_helm_install.shcommon/dex/helmci/values-oauth2-proxy.yamltests/oauth2-proxy_helm_install.shcommon/oauth2-proxy/helmci/values-m2m-dex-and-kind.yamltests/dashboard_helm_install.shapplications/dashboard/helmci/values-platform.yamltests/notebooks_helm_install.shapplications/notebooks-v1/helmci/values-platform.yamltests/katib_helm_install.shexperimental/helm/charts/katibci/values-kubeflow.yamlEvery install uses the ci values file of that chart's comparison scenario, so the installed manifests are exactly the ones
tests/run_helm_kustomize_comparison.pyproves equivalent to the Kustomize target the original step applied — including the parity fixture credentials the login test depends on. Istio stays onistio-cni_install.shbecause the chart's compared scenarios cover the sidecar profile, not the CNI profile the gate installs; that swap needs its own decision.Growth model
A component's Helm install step joins this workflow in the pull request that adds its chart — the same ownership model as
ci/comparison.yaml. The Pipelines chart (#3552, which already carriestests/pipelines_helm_install.sh) and the Spark Operator chart (#3575) each add their one-step swap once this merges. When this workflow has run stably alongside the Kustomize gate, replacingfull_kubeflow_integration_test.yamlin place is a small, separately approved follow-up.Triggers
workflow_dispatch, pluspull_requestfiltered to chart paths, the Helm install scripts, and this workflow file — so the second full-platform job does not load every pull request while it is being proven.Verification
helm template <release> <chart> --namespace <n> --values <ci values>renders under Helm 4.2.2 (nine commands, including both cert-manager phases).bash -non all eight scripts; workflow parses.