lupyuen commented on issue #18359: URL: https://github.com/apache/nuttx/issues/18359#issuecomment-3883079521
_Earlier we talked about reimplementing the Official GitHub Labeler `actions/labeler`, so that we can do the PR Labeling safely. Any updates?_ Yep it works! Here's a PR that was correctly identified and labeled as an Arm64 PR: https://github.com/lupyuen6/nuttx/pull/22 > <img width="922" height="674" alt="Image" src="https://github.com/user-attachments/assets/616777b9-b4f3-49c2-b495-d7072cc3bfc7"; /> Which will correctly trigger an Arm64 Build. So we're almost done! Just needs more testing... https://github.com/lupyuen6/nuttx/actions/runs/21896055449 > <img width="2003" height="990" alt="Image" src="https://github.com/user-attachments/assets/13bb5315-52f5-4a6e-91b5-4f97fa03ee14"; /> _How does it work?_ Remember how the old labeler.yml will Checkout the Entire NuttX Repo based on the PR? This is kinda dangerous, because it might checkout Malicious Code from the PR, and accidentally execute the Malicious Code! In the new labeler.yml: We checkout only One Single File... the Labeler Config `.github/labeler.yml`, direct from the Trusted Source (i.e. Official NuttX Repo) https://github.com/lupyuen6/nuttx/blob/master/.github/workflows/labeler.yml <img width="1153" height="1051" alt="Image" src="https://github.com/user-attachments/assets/55f5a395-dac9-487f-8a76-a2803d1dcb35"; /> We call GitHub API to fetch the PR Details: (1) Filenames of the Changed Files (2) How Many Lines Were Changed In Each File... ```text { filename: 'arch/arm/test.txt', status: 'added', additions: 3, deletions: 0, changes: 3 } ``` https://github.com/lupyuen6/nuttx/blob/master/.github/workflows/labeler.yml <img width="1502" height="945" alt="Image" src="https://github.com/user-attachments/assets/2b89d4ee-317b-48fc-9580-cd3c465fe524"; /> (We don't actually need the Entire NuttX Repo based on the PR!) We parse the Labeling Rules in `.github/labeler.yml`. We won't implement the Entire GitHub Labeler `actions/labeler`, just the bare minimum needed for NuttX... https://github.com/lupyuen6/nuttx/blob/master/.github/workflows/labeler.yml <img width="1524" height="1416" alt="Image" src="https://github.com/user-attachments/assets/47f2c904-388e-4ce0-bec5-aa6f71becaec"; /> Then we apply the Labeling Rules to get the PR Labels... https://github.com/lupyuen6/nuttx/blob/master/.github/workflows/labeler.yml <img width="1258" height="1160" alt="Image" src="https://github.com/user-attachments/assets/3add4fa3-0d1d-4511-92b5-c0edc5f0272d"; /> Finally: The PR Labels are written safely to the PR in the workflow_run trigger... https://github.com/lupyuen6/nuttx/blob/master/.github/workflows/pr_labeler.yml <img width="2413" height="982" alt="Image" src="https://github.com/user-attachments/assets/8290d1ff-2c58-4686-8ec9-9ad673ece512"; /> Up Next: I'll explore the PR Size Label. It might be easy, because GitHub API already counts the changed lines for us. _Pros and Cons of the new implementation?_ (1) __New Implementation is Safer:__ We don't use pull_request_target, and we don't checkout the Entire NuttX Repo based on the PR. So we'll never execute any Malicious Code submitted in the PR. (2) __New Implementation is Quicker:__ It's faster since we don't checkout the entire repo. Also `pr-size-labeler` actually runs in a Docker Container, we don't need that any more. (3) __But it might be quirky under Heavy Load__. Remember that workflow_run trigger will write the PR Labels as a Second Job? When we run out of GitHub Runners, the PR Labels might never be applied. The Build Logic in arch.yml will execute a Complete NuttX Build if it can't find the PR Labels. (4) Will the Build Workflow be triggered too early, before the workflow_run trigger? Hopefully not. The Build Workflow begins in the Fetch-Source stage, checking out the Entire Repo and uploading everything in 1.5 minutes, followed by the Select-Builds stage (arch.yml) reading the PR Labels. Before 1.5 minutes, rightfully our workflow_run trigger would have written the PR Labels to the PR. _Are we reimplementing EVERYTHING from the Official GitHub Labeler `actions/labeler`?_ We won't implement the Entire GitHub Labeler `actions/labeler`, just the bare minimum needed for NuttX. We're emulating the Labeler Config `.github/labeler.yml` as is, because someday GitHub might invent a Secure Way to Label PRs inside pull_request_trigger. (Then we'll switch back to the Official GitHub Labeler `actions/labeler`) There's something really jinxed about the way GitHub designed PR Labeling in pull_request_trigger, it's a terrible security hack. I'll write an article about this someday :-) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
