Skip to content

ci: add upstream folder change check#3431

Open
christian-heusel wants to merge 1 commit into
kubeflow:masterfrom
christian-heusel:ci/add-upstream-check
Open

ci: add upstream folder change check#3431
christian-heusel wants to merge 1 commit into
kubeflow:masterfrom
christian-heusel:ci/add-upstream-check

Conversation

@christian-heusel

@christian-heusel christian-heusel commented Apr 1, 2026

Copy link
Copy Markdown
Member

Pull Request Template for Kubeflow Manifests

✏️ Summary of Changes

  • ci: Add workflow to detect upstream folder changes in PRs

See christian-heusel/community-distribution-playground#3 (review) for a test of this new Github Action 🤗

📦 Dependencies

  • addition of a upstream-changed label

🐛 Related Issues

none

✅ Contributor Checklist

  • I have tested these changes with kustomize. See Installation Prerequisites.
  • 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, since I expect help from the community for my issue (see 1. and 2.).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a GitHub Actions workflow to detect and warn about changes to upstream folders in pull requests. The workflow triggers on pull requests that modify files in any **/upstream/** path and outputs a warning if such changes are detected.

Changes:

  • Added .github/workflows/check_upstream_changes.yaml workflow that detects modifications to upstream folder structures
  • Added test string "Test123" to applications/volumes-web-app/upstream/base/cluster-role-binding.yaml

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
.github/workflows/check_upstream_changes.yaml New workflow to detect upstream folder changes and emit warnings
applications/volumes-web-app/upstream/base/cluster-role-binding.yaml Unintended test content added

Comment thread applications/volumes-web-app/upstream/base/cluster-role-binding.yaml Outdated
Comment thread .github/workflows/check_upstream_changes.yaml Outdated
@christian-heusel christian-heusel force-pushed the ci/add-upstream-check branch 3 times, most recently from b5a84ca to a3cb231 Compare April 1, 2026 17:55
@christian-heusel christian-heusel changed the title ci: add upstream check ci: add upstream folder change check Apr 1, 2026
@christian-heusel christian-heusel marked this pull request as ready for review April 1, 2026 17:55
@juliusvonkohout juliusvonkohout requested a review from Copilot April 4, 2026 10:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.

Comment thread .github/workflows/check_upstream_changes.yaml Outdated
Comment thread .github/workflows/check_upstream_changes.yaml Outdated
Comment thread .github/workflows/check_upstream_changes.yaml Outdated
Comment thread .github/workflows/check_upstream_changes.yaml Outdated
@juliusvonkohout

Copy link
Copy Markdown
Member

I think relying on #3464 is easier and faster, but please reopen if needed.
/close

@google-oss-prow google-oss-prow Bot closed this May 16, 2026
@google-oss-prow

Copy link
Copy Markdown

@juliusvonkohout: Closed this PR.

Details

In response to this:

I think relying on #3464 is easier and faster, but please reopen if needed.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@thesuperzapper

thesuperzapper commented Jun 24, 2026

Copy link
Copy Markdown
Member

@christian-heusel I think we should aim to use PR checks rather than hoping the review agent detects it (and how could it know what the "true" upstream state is, as you would need to check it by checking out the upstream).

I know this PR will need a lot of rebasing, but I am going to reopen so we can track.

/reopen

@google-oss-prow google-oss-prow Bot reopened this Jun 24, 2026
@google-oss-prow

Copy link
Copy Markdown

@thesuperzapper: Reopened this PR.

Details

In response to this:

@christian-heusel I think we should aim to use PR checks rather than hoping the agent detects it (and how could it know what the "true" upstream state is, as you would need to check it by checking out the upstream).

I know this PR will need a lot of rebasing, but I am going to reopen so we can track.

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@google-oss-prow

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kimwnasptd for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

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

@google-oss-prow google-oss-prow Bot added size/L and removed size/M labels Jun 24, 2026
@christian-heusel christian-heusel marked this pull request as ready for review June 24, 2026 05:35
@google-oss-prow google-oss-prow Bot requested a review from tarekabouzeid June 24, 2026 05:35
@thesuperzapper

Copy link
Copy Markdown
Member

This PR must be updated to not use pull_request_target.

In its current form, it is a critical supply chain risk.

/hold

@thesuperzapper

Copy link
Copy Markdown
Member

Nothing about this problem requires anything but the remote branch, and a trigger on only specific paths for PRs.

What we are checking is if the upstream folder is literally the exact one referenced in some control file (probably called GIT_REFERENCE in each upstream parent folder).

Adds a `pull_request_target` workflow that triggers whenever a pull
request touches files under any `upstream/` folder.

The workflow:
- Applies the
    - `upstream-changed` label to the pull request if there are changed
      files
    - `hold/do-not-merge` label to the pull request for outside
      contributors
- Posts a pull request review with a GitHub markdown alert warning with
    - "Request changes" for pull requests opened from forks by
      non-organization members
    - "Comment" otherwise (GitHub does not allow the repository token to
      submit REQUEST_CHANGES reviews on pull requests from within the
      same repository)

Assisted-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Christian Heusel <christian@heusel.eu>
@christian-heusel

christian-heusel commented Jun 24, 2026

Copy link
Copy Markdown
Member Author

@thesuperzapper I have now dropped the code path that would potentially handle untrusted input (the filenames), I think you can unhold the PR.

I'll work on a separate approach for this topic as we discussed via PM, however until then this is what I can offer 🤗

@thesuperzapper

Copy link
Copy Markdown
Member

There is no reason why this check needs to be done using pull request target, and it is not safe to do so.

We must not merge this PR until a new approach is taken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants