scripts: fix follow-up issues from #3371

[danish9039]

✏️ Summary of Changes

This PR follows up on review feedback from #3371 and fixes the remaining correctness and portability issues in the sync scripts.

Original PR:

• Standardize scripts #3371

Changes included:

• fix create_branch in scripts/library.sh by removing the fragile empty-test branch check
• fix scripts/synchronize-kubeflow-manifests.sh so the CRUD web app source paths match the current upstream Kubeflow layout:
  • components/crud-web-apps/jupyter/manifests
  • components/crud-web-apps/volumes/manifests
  • components/crud-web-apps/tensorboards/manifests
• fix the CRUD web app README replacement paths in scripts/synchronize-kubeflow-manifests.sh
• narrow commit_changes usage so the Kubeflow and Knative sync scripts stage only their intended outputs instead of the whole repo
• make replace_in_file() in scripts/synchronize-knative-manifests.sh Darwin-safe

Review comments addressed:

• scripts/library.sh: fragile branch existence check
  • Standardize scripts #3371 (comment)
• scripts/synchronize-kubeflow-manifests.sh: CRUD web app README replacement mismatch
  • Standardize scripts #3371 (comment)
• scripts/library.sh: overly broad staging through commit_changes
  • Standardize scripts #3371 (comment)
• scripts/synchronize-knative-manifests.sh: non-portable sed -i
  • Standardize scripts #3371 (comment)

📦 Dependencies

• Follow-up to #3371

🐛 Related Issues

• Addresses remaining review feedback on #3371

✅ Contributor Checklist

• I have tested these changes locally.
• All commits are signed-off to satisfy the DCO check.
• I have considered adding my company to the adopters page to support Kubeflow and help the community for my issue.

Local Validation

Validated in a clean worktree and disposable repos:

• bash -n on all modified scripts
• actual run of scripts/synchronize-kubeflow-manifests.sh after fixing CRUD source paths
• actual run of scripts/synchronize-knative-manifests.sh with local yq available
• explicit check that only intended files are staged/committed
• mocked Darwin path for replace_in_file() using sed -i ''

[github-actions]

Welcome to the Kubeflow Manifests Repository

Thanks for opening your first PR. Your contribution means a lot to the Kubeflow community.

Before making more PRs:
Please ensure your PR follows our Contributing Guide.
Please also be aware that many components are synchronizes from upstream via the scripts in /scripts.
So in some cases you have to fix the problem in the upstream repositories first, but you can use a PR against kubeflow/manifests to test the platform integration.

Community Resources:

• CNCF Slack channels: https://www.kubeflow.org/docs/about/community/#kubeflow-slack-channels
• Community meetings: https://www.kubeflow.org/docs/about/community/

Thanks again for helping to improve Kubeflow.

[Copilot]

Pull request overview

This PR is a follow-up to #3371 and fixes several correctness and portability issues in the Kubeflow manifest synchronization scripts. It corrects incorrect CRUD web app source paths, aligns the sed -i usage in the Knative script to be Darwin-safe, removes a fragile/redundant branch existence check, and narrows the git add scope in commit_changes calls so only the intended files are staged rather than the entire repository.

Changes:

• Remove fragile empty-test branch check in create_branch (scripts/library.sh)
• Fix CRUD web app source/destination paths (crud-web-applications → crud-web-apps) and narrow commit_changes staging to applications/ and README.md in scripts/synchronize-kubeflow-manifests.sh
• Add Darwin-safe sed -i '' handling and narrow commit_changes staging in scripts/synchronize-knative-manifests.sh

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
scripts/library.sh	Removes the git branch --list empty-test check that was shadowed by the more reliable git show-ref check immediately after it
scripts/synchronize-kubeflow-manifests.sh	Corrects upstream CRUD web app source and README replacement paths from crud-web-applications to crud-web-apps; narrows commit_changes to stage only applications/ and the top-level README.md
scripts/synchronize-knative-manifests.sh	Adds macOS-compatible sed -i '' branch in replace_in_file; narrows commit_changes to stage only $DESTINATION_DIRECTORY and the top-level README.md

[juliusvonkohout]

Thank you.
May you check whether all the extra stuff for Knative is still needed and whether we can simplify it?
Please remove all the Darwin stuff. The target is a proper Linux.

[danish9039]

Thank you. May you check whether all the extra stuff for Knative is still needed and whether we can simplify it? Please remove all the Darwin stuff. The target is a proper Linux.

@juliusvonkohout done !

[juliusvonkohout]

Please check whether all the yq eval -i 'explode(.)' for knative is still needed with kustomize 5.8.1 or whether we can drop it.

[danish9039]

Please check whether all the yq eval -i 'explode(.)' for knative is still needed with kustomize 5.8.1 or whether we can drop it.

@juliusvonkohout I checked this locally against kustomize v5.8.1 and ran the Knative sync flow twice in disposable trees

• once with the current yq eval -i 'explode(.)' lines
• once with all seven explode(.) lines removed

Then I compared the generated Knative YAML and ran kustomize build on

• common/knative/knative-serving/base
• common/knative/knative-eventing/base
• common/knative/knative-serving-post-install-jobs/base
• common/knative/knative-eventing-post-install-jobs/base

The generated files were identical, and all four builds passed without explode(.), so it is not needed anymore. I removed those lines in the latest commit .

[juliusvonkohout]

Please check whether all the yq eval -i 'explode(.)' for knative is still needed with kustomize 5.8.1 or whether we can drop it.

@juliusvonkohout I checked this locally against kustomize v5.8.1 and ran the Knative sync flow twice in disposable trees

* once with the current `yq eval -i 'explode(.)'` lines

* once with all seven `explode(.)` lines removed

Then I compared the generated Knative YAML and ran kustomize build on

* `common/knative/knative-serving/base`

* `common/knative/knative-eventing/base`

* `common/knative/knative-serving-post-install-jobs/base`

* `common/knative/knative-eventing-post-install-jobs/base`

The generated files were identical, and all four builds passed without explode(.), so it is not needed anymore. I removed those lines in the latest commit .

I still see a lot of yq eval in the script. Please check what you can remove.

[juliusvonkohout]

Or let us tackle #3375 (comment) in a follow up PR.
/lgtm
/approve

[google-oss-prow]

[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

Details

Needs approval from an approver in each of these files:

• OWNERS [juliusvonkohout]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment