Standardize scripts#3371
Conversation
Co-authored-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>
Co-authored-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>
/scripts synchronization scripts around the shared template contract
Co-authored-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>
Co-authored-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>
Co-authored-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>
Co-authored-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>
update_readme logging change to restore pre-PR script output behavior
Co-authored-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>
update_readme logging change to restore pre-PR script output behaviorCo-authored-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: juliusvonkohout The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR standardizes the Kubeflow manifests synchronization shell scripts to use shared helpers (library.sh) consistently, while aiming to keep on-disk output behavior identical to master.
Changes:
- Simplifies and normalizes synchronization script structure (variable naming, whitespace, and shared helper usage).
- Replaces per-script copy/README/commit logic with
copy_manifests,update_readme, and a unifiedcommit_changescall pattern. - Refactors a few scripts to reduce inline
sed/copy logic in favor of common library functions.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/library.sh | Common helper cleanup and used as the standardized mechanism for copy/README update/commit operations. |
| scripts/template.sh | Updates the template to match the new standardized helper usage pattern. |
| scripts/synchronize-cert-manager-manifests.sh | Switches README updates to update_readme and standardizes commit flow. |
| scripts/synchronize-dex-manifests.sh | Switches README updates to update_readme and standardizes commit flow. |
| scripts/synchronize-istio-manifests.sh | Standardizes variables/paths and README update + commit flow. |
| scripts/synchronize-katib-manifests.sh | Standardizes README update + commit flow. |
| scripts/synchronize-knative-manifests.sh | Standardizes variables/commit flow and keeps custom README replacements. |
| scripts/synchronize-kserve-kserve-manifests.sh | Standardizes variables/paths and README update + commit flow. |
| scripts/synchronize-kserve-web-application-manifests.sh | Standardizes README update + commit flow. |
| scripts/synchronize-kubeflow-manifests.sh | Refactors component copy helper signature and standardizes commit flow. |
| scripts/synchronize-model-registry-manifests.sh | Replaces inline copy logic with copy_manifests and standardizes commit flow. |
| scripts/synchronize-oauth2-proxy-manifests.sh | Switches README updates to update_readme and standardizes commit flow. |
| scripts/synchronize-pipelines-manifests.sh | Standardizes README update + commit flow. |
| scripts/synchronize-spark-operator-manifests.sh | Standardizes version variables and commit flow; keeps custom README update via sed. |
| scripts/synchronize-trainer-manifests.sh | Standardizes README update + commit flow. |
| scripts/synchronize-training-operator-manifests.sh | Standardizes README update + commit flow. |
| create_branch() { | ||
| local branch="$1" | ||
|
|
||
| check_uncommitted_changes | ||
|
|
||
| if [ $(git branch --list "$branch") ]; then | ||
| echo "WARNING: Branch $branch already exists." | ||
| fi | ||
|
|
||
| if ! git show-ref --verify --quiet refs/heads/$branch; then |
There was a problem hiding this comment.
create_branch uses if [ $(git branch --list "$branch") ]; then ..., which can emit a test error when the branch does not exist because the command substitution expands to an empty string (resulting in [ ]). Use a string test with proper quoting (e.g., if [[ -n $(git branch --list "$branch") ]]; then) or a command that directly checks existence (git show-ref --verify --quiet "refs/heads/$branch").
| copy_component_manifests "components/crud-web-applications/jupyter/manifests" \ | ||
| "applications/jupyter/jupyter-web-app/upstream" \ | ||
| "crud-web-applications/jupyter/manifests" | ||
|
|
||
| copy_component_manifests "volumes-web-app" \ | ||
| "components/crud-web-applications/volumes/manifests" \ | ||
| copy_component_manifests "components/crud-web-applications/volumes/manifests" \ | ||
| "applications/volumes-web-app/upstream" \ | ||
| "crud-web-applications/volumes/manifests" | ||
|
|
||
| copy_component_manifests "tensorboards-web-app" \ | ||
| "components/crud-web-applications/tensorboards/manifests" \ | ||
| copy_component_manifests "components/crud-web-applications/tensorboards/manifests" \ | ||
| "applications/tensorboard/tensorboards-web-app/upstream" \ | ||
| "crud-web-applications/tensorboards/manifests" |
There was a problem hiding this comment.
The README link replacement patterns for the CRUD web apps use crud-web-applications/..., but the root README.md currently references components/crud-web-apps/... (no "-applications"). As a result, these update_readme calls will not update the corresponding rows when COMMIT changes. Consider broadening the match (handle both paths) or aligning the patterns to the current README paths so the version table stays in sync.
|
/lgtm |
This addresses the request to verify that
copilot/standardize-scripts-formatis behavior-preserving: run the modified scripts and confirm they produce the same on-disk results asmaster.The outcome is parity: no branch-vs-master output drift was found for the modified synchronization scripts.
Scope
scripts/*.shfiles againstmaster(16 modified files total).scripts/synchronize-*.sh, 14 scripts) and compared filesystem deltas for each run.Environment parity adjustments (non-repo)
ruamel.yamlResult
branch_exit == master_exitandbranch_changed == master_changedfor every runnable modified script.masterfor all compared runs.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
kubeflow.github.io/usr/local/bin/helm helm template -n kubeflow --include-crds spark-operator spark-operator --set spark.jobNamespaces={} --set webhook.enable=true --set webhook.port=9443 --version 2.4.0 --repo REDACTED 3/runs/synchroni/tmp/script-output-equivalence-v3/runs/synchronize-pipelines-manifests/master/.tsynchronize-spark-operator-manifests-2.4.0 ests/master/.toostatus bash(dns block)/usr/local/bin/helm helm template -n kubeflow --include-crds spark-operator spark-operator --set spark.jobNamespaces={} --set webhook.enable=true --set webhook.port=9443 --version 2.4.0 --repo REDACTED ests/branch/READ/tmp/script-output-equivalence-v3/runs/synchronize-spark-operator-manifests/bransynchronize-spark-operator-manifests-2.4.0 ndor/bin/bash bash(dns block)If you need me to access, download, or install something from one of these locations, you can either:
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.