You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "dongjoon-hyun (via GitHub)" <gi...@apache.org> on 2026/10/21 18:03:34 UTC

[PR] [SPARK-60440] Migrate `notify_test_workflow.yml` from `pull_request_target` to `workflow_run` [spark-connect-swift]

dongjoon-hyun opened a new pull request, #615:
URL: https://github.com/apache/spark-connect-swift/pull/615

   ### What changes were proposed in this pull request?
   
   This PR aims to migrate `notify_test_workflow.yml` from `pull_request_target` to the signal and `workflow_run` pattern of the [ASF GitHub Actions Policy](https://infra.apache.org/github-actions-policy.html), like apache/spark-kubernetes-operator#976.
   
   - Add `notify_test_workflow_signal.yml`, an unprivileged `pull_request` workflow with `permissions: {}` and a single `echo` step, for the same `opened`, `reopened` and `synchronize` events. It takes over the general name `On pull request update` and its comment, because the `Build` check is attached to a check suite on the PR head commit, which is now the suite of this run. It uses `ubuntu-slim`, like `notify_test_workflow.yml`.
   - Change `notify_test_workflow.yml` (now named `Notify test workflow`) to run on `workflow_run` of `On pull request update`. GitHub runs it from the default branch, so it never runs the code of a PR. It checks `github.event.workflow_run.event == 'pull_request'`, sets `permissions: {}` at the top level, and keeps `actions: read` and `checks: write` for the job. The script reads the head repository, branch and SHA from `workflow_run` instead of `pull_request.head`.
   
     | `pull_request_target` | `workflow_run` |
     | --- | --- |
     | `pull_request.head.repo.owner.login` | `workflow_run.head_repository.owner.login` |
     | `pull_request.head.repo.name` | `workflow_run.head_repository.name` |
     | `pull_request.head.repo.full_name` | `workflow_run.head_repository.full_name` |
     | `pull_request.head.ref` | `workflow_run.head_branch` |
     | `pull_request.head.sha` | `workflow_run.head_sha` |
   
   - Since the `workflow_run` run belongs to the `main` commit, the PR does not show it when it fails. So, a new `Report failure` step (`if: failure()`) creates a `completed` / `action_required` `Build` check on the head commit with a link to the failed run. The check run lookup is also made best-effort, as its comment says, so a lookup failure falls back to the Actions URL instead of failing the job.
   - `build_main.yml`, `build_and_test.yml` and `update_build_status.yml` are unchanged. The `Build` check keeps its name and `output.text` (`{owner, repo, run_id}`), so the scheduled updater keeps syncing it and skips `action_required` ones as before.
   
   ```mermaid
   flowchart TD
       subgraph fork["Forked repo (contributor)"]
           P([push to any branch]) --> BM["build_main.yml — job: Run"]
           BM -->|workflow_call| BAT["build_and_test.yml (reusable)"]
           BAT --> CR["check runs:<br/>Run / License Check, ..."]
       end
   
       subgraph up["Upstream repo (apache/spark-connect-swift)"]
           PR([pull_request]) --> S["notify_test_workflow_signal.yml<br/>On pull request update, no permissions"]
           S -->|"workflow_run (requested)"| N["notify_test_workflow.yml<br/>runs from main, checks: write"]
           SCH([schedule: every 15 min]) --> U["update_build_status.yml"]
           N -->|create| B(["Build check on PR"])
           U -->|"PATCH status / conclusion"| B
       end
   
       BM -.->|"① find run (id: build_main.yml)"| N
       CR -.->|"② link check-run view"| N
       BM -.->|"③ poll run status"| U
   ```
   
   `notify_test_workflow.yml` uses `types: [requested]` instead of `[completed]`. By default, ASF requires approval for every `pull_request` run from external contributors. GitHub creates such a run as `completed` / `action_required` without jobs and sends `requested` for it at once, but it sends `completed` only after a committer approves the run and the run finishes. `requested` is not sent again when a run is approved or re-run. So, every push gets exactly one `Build` check without approval, as it does now.
   
   GitHub does not run `pull_request` workflows while a PR has merge conflicts, so such a PR gets a new `Build` check after the conflicts are resolved.
   
   ### Why are the changes needed?
   
   The ASF GitHub Actions Policy says, "From November 2026, the pull_request_target trigger is disabled for ASF repositories." Without this PR, PRs from forks get no `Build` check from November 2026.
   
   ### Does this PR introduce _any_ user-facing change?
   
   No. This only changes the CI workflows that maintain the `Build` status check of PRs.
   
   ### How was this patch tested?
   
   - `yq` parses all workflow files. `workflow_run.workflows` matches the name of the signal workflow, and no `pull_request_target` remains under `.github`.
   - Both `github-script` bodies of `notify_test_workflow.yml` compile as async functions, the way `actions/github-script` runs them.
   - Both workflow files are identical to those of apache/spark-kubernetes-operator after SPARK-60408 except the runner label (`ubuntu-slim`), the project name and the image URL.
   - The whole flow takes effect only after the merge to `main`, because GitHub runs `workflow_run` workflows from the default branch.
   
   ### Was this patch authored or co-authored using generative AI tooling?
   
   Generated-by: Claude Opus 5.5


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-60440] Migrate `notify_test_workflow.yml` from `pull_request_target` to `workflow_run` [spark-connect-swift]

Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #615: [SPARK-60440] Migrate `notify_test_workflow.yml` from `pull_request_target` to `workflow_run`
URL: https://github.com/apache/spark-connect-swift/pull/615


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-60440] Migrate `notify_test_workflow.yml` from `pull_request_target` to `workflow_run` [spark-connect-swift]

Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #615:
URL: https://github.com/apache/spark-connect-swift/pull/615#discussion_r4340658221


##########
.github/workflows/notify_test_workflow.yml:
##########
@@ -188,11 +191,31 @@ jobs:
                     + 'If the failures are related to this pull request, '
                     + 'please investigate them and push follow-up changes.',
                   text: JSON.stringify({
-                    owner: context.payload.pull_request.head.repo.owner.login,
-                    repo: context.payload.pull_request.head.repo.name,
+                    owner: context.payload.workflow_run.head_repository.owner.login,
+                    repo: context.payload.workflow_run.head_repository.name,
                     run_id: run_id
                   })
                 },
                 details_url: actions_url,
               })
             }
+      - name: "Report failure"

Review Comment:
   With this step, the `any_runs && !matched_run` branch (L98-101, outside the diff) now ends with an `action_required` `Build` check on this SHA, so its comment ("a fresh notify run will handle it; nothing useful to report for this SHA") is no longer accurate. As you found in the operator, those failures were commits the fork never built rather than newer pushes, and a contributor cannot retrigger this run, so "Please retrigger the workflow" doesn't help them either. Could we update that comment and message?
   
   Also, since this is a separate step, it can't see the error, and the summary only says the detection failed. For the never-built case, the guidance from the `!matched_run` branch (enable GitHub Actions in the fork, sync with the latest `main`) is what the contributor needs. Would it make sense to add a short version of it here?



##########
.github/workflows/notify_test_workflow_signal.yml:
##########
@@ -0,0 +1,38 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+# Intentionally has a general name.
+# because the test status check created in GitHub Actions
+# currently randomly picks any associated workflow.
+# So, the name was changed to make sense in that context too.
+# See also https://github.community/t/specify-check-suite-when-creating-a-checkrun/118380/10
+name: On pull request update

Review Comment:
   `notify_test_workflow.yml` is triggered by this exact name via `workflow_run.workflows`, so renaming this workflow would silently stop the `Build` check. Maybe worth a note next to the existing comment:
   
   ```suggestion
   # notify_test_workflow.yml is triggered by this name via `workflow_run.workflows`.
   name: On pull request update
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-60440] Migrate `notify_test_workflow.yml` from `pull_request_target` to `workflow_run` [spark-connect-swift]

Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #615:
URL: https://github.com/apache/spark-connect-swift/pull/615#issuecomment-6324073259

   Thank you for the review and approval, @viirya. Yes, the approval path is not exercised yet in either repository. I'll check the first external PR after the merge, especially whether the `Build` check stays visible after the signal run is approved. I'll also make the same changes in the operator via a follow-up so that the two files stay identical.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-60440] Migrate `notify_test_workflow.yml` from `pull_request_target` to `workflow_run` [spark-connect-swift]

Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #615:
URL: https://github.com/apache/spark-connect-swift/pull/615#discussion_r4340752127


##########
.github/workflows/notify_test_workflow.yml:
##########
@@ -188,11 +191,31 @@ jobs:
                     + 'If the failures are related to this pull request, '
                     + 'please investigate them and push follow-up changes.',
                   text: JSON.stringify({
-                    owner: context.payload.pull_request.head.repo.owner.login,
-                    repo: context.payload.pull_request.head.repo.name,
+                    owner: context.payload.workflow_run.head_repository.owner.login,
+                    repo: context.payload.workflow_run.head_repository.name,
                     run_id: run_id
                   })
                 },
                 details_url: actions_url,
               })
             }
+      - name: "Report failure"

Review Comment:
   Thank you. You're right. Since SPARK-57156, the run is looked up by the head SHA, so a newer push no longer leads to this branch. I updated the comment and the error message, and added short guidance to the `Report failure` summary.



##########
.github/workflows/notify_test_workflow_signal.yml:
##########
@@ -0,0 +1,38 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+# Intentionally has a general name.
+# because the test status check created in GitHub Actions
+# currently randomly picks any associated workflow.
+# So, the name was changed to make sense in that context too.
+# See also https://github.community/t/specify-check-suite-when-creating-a-checkrun/118380/10
+name: On pull request update

Review Comment:
   Thank you. Added.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-60440] Migrate `notify_test_workflow.yml` from `pull_request_target` to `workflow_run` [spark-connect-swift]

Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #615:
URL: https://github.com/apache/spark-connect-swift/pull/615#issuecomment-6324110717

   Merged to main


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org