helm: add Kubeflow Pipelines chart - #3552
Conversation
4c9d813 to
104e494
Compare
|
[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 |
104e494 to
d17a0dd
Compare
6664282 to
c027ee9
Compare
91d3436 to
12c852e
Compare
|
/retest |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 34 changed files in this pull request and generated no new comments.
Suppressed comments (4)
.github/workflows/helm-kustomize-comparison.yml:175
- This step never executes the two newly added test modules, and no other GitHub Actions workflow invokes them. Consequently, their assertions can fail without affecting continuous integration. Run both modules here.
- name: Test Kubeflow Pipelines Helm behavior
run: |
python tests/test_pipelines_helm_chart.py
python -m unittest tests/test_generate_pipelines_helm_manifests.py
.github/workflows/helm-kustomize-comparison.yml:28
- The two newly added test modules are absent from this path filter, so a later pull request that changes only either module will not start this workflow. Add both paths.
This issue also appears on line 172 of the same file.
- tests/test_pipelines_helm_chart.py
- tests/test_generate_pipelines_helm_manifests.py
applications/pipeline/helm/README.md:90
- This command uses Helm's default release namespace, but
templates/validate-namespace.yamlrejects every namespace exceptkubeflow. It therefore does not lint the chart under its supported configuration; add the required namespace, as done inapplications/dashboard/helm/README.md:203.
helm lint applications/pipeline/helm
.github/workflows/helm-pipelines-test.yaml:11
- This job installs Istio from
common/istio, OAuth2-Proxy fromcommon/oauth2-proxy, and the Profile Controller fromapplications/dashboard/upstream/profile-controller, but none of those inputs is included in the filter. Changes to these installation dependencies can therefore break this scenario without running the workflow. Add their paths.
- common/cert-manager/**
- common/kubeflow-namespace/**
- common/kubeflow-roles/**
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
12c852e to
7bd92e1
Compare
Pull Request Template for Kubeflow Manifests
✏️ Summary of Changes
Adds the Kubeflow Pipelines Helm chart under
applications/pipeline/helm,co-located with the Kustomize component it wraps.
Kustomize stays the source of truth. The synchronization script builds both
supported overlays and writes deterministic payloads under
manifests/, whichsmall templates load with
.Files.Get. Helm does not evaluate that content, sothe Argo expressions inside upstream manifests survive verbatim.
Two scenarios, matching the two installable variants:
platform-databaseapplications/pipeline/overlaysplatform-k8s-nativeapplications/pipeline/upstream/env/cert-manager/platform-agnostic-multi-user-k8s-nativeHow it works
flowchart TD A["applications/pipeline/overlays"] --> C["scripts/generate-pipelines-helm-manifests.py"] B["upstream/env/cert-manager/<br/>platform-agnostic-multi-user-k8s-native"] --> C C --> D["validate identity,<br/>no duplicates"] D --> E["partition: common,<br/>database-only, k8s-native-only"] E --> F["CustomResourceDefinition?<br/>resource-policy keep"] F --> G["manifests/*.yaml<br/>six payloads, plain YAML"] G --> H["templates/*.yaml<br/>one 3-line switch per payload"] H --> I["rendered output equals kustomize build"]Payloads are not templates
Kubeflow Pipelines manifests embed Argo expressions such as
{{workflow.name}}.While the payloads lived under
templates/, Helm evaluated them, so everydelimiter had to be escaped on the way out and unescaped by the template engine
on the way back. A missed escape corrupts a manifest silently, and the committed
payload was not a faithful record of the render.
They now live in
manifests/and are read with.Files.Get, which Helm does notevaluate.
escape_helm_delimitersand the 13 escaped delimiters are gone.One payload per template file. Two
.Files.Getincludes in a single templatefile concatenate without a document separator, which merges the last resource of
one payload into the first of the next. This was caught by the Kustomize
comparison during development — a
ClusterRolehad acquired aCustomResourceDefinition'sspec, and the result was still valid YAML.The
{{- if }}conditions also moved out of the generated files intotemplates/, so the generator no longer decides when a payload applies.Release namespace corrected to
kubeflowThis changes the documented installation command and deserves a maintainer's
opinion.
Every namespaced resource this chart renders declares
namespace: kubeflow, butthe chart, its README, the comparison scenarios and the end-to-end install script
all used
kubeflow-systemas the release namespace. A release installedelsewhere stores its metadata in one namespace while modifying another, and
helm uninstallthen deletes resources it does not appear to own — the reasoningthe Dashboard chart README already documents.
templates/validate-namespace.yamlnow enforceskubeflow, matching the otherwrapper charts. Reverting this is a one-line change if you disagree.
Tests
tests/test_pipelines_helm_workflows.pyis removed. It asserted that a workflowfile contains particular strings, which mirrors configuration rather than
checking behaviour: it fails only when someone edits the workflow, and the fix is
to edit the test. Four of its five assertions describe things a failing workflow
run reports anyway.
The tests that do check behaviour ran in no workflow at all. They now run in
the comparison workflow's
chart-behavior-testsjob, which already has Helm andKustomize available, under the component-first file names (
pipelines_helm_chart_test.py,pipelines_helm_manifest_generator_test.py,pipelines_helm_install_helper_test.py).Still to come, deliberately not here
The
commonpartition stays for now. Dropping it is part of the shared-enginework that has to wait until #3524 and #3525 merge, because both carry
scripts/helm_manifest_generator.pyand three divergent copies would be worsethan one duplicate generator.
📦 Dependencies
Depends on #3524 for
scripts/helm_manifest_generator.py, which this chart willadopt once that merges. Until then it keeps its own generator.
The comparison harness (#3577)
is merged, and this chart registers through its own
applications/pipeline/helm/ci/comparison.yaml; no shared comparison file istouched. The descriptor declares the sixteen retained CustomResourceDefinitions
the generator annotates with
helm.sh/resource-policy: keep.🐛 Related Issues
Part of the Project 5 Helm chart work.
Validation
Verified locally against Helm 4.1.0 and Kustomize v5.8.1.
✅ Contributor Checklist