LibreSolar / bms-c1

16s / 100A Battery Management System
https://libre.solar/bms-c1/manual/
Other
120 stars 38 forks source link

ci: Add PDF-diff commenting to PR workflow #52

Closed pasrom closed 10 months ago

pasrom commented 10 months ago

This PR is a followup of https://github.com/LibreSolar/bms-c1/pull/48 to implement the task "Running diff tools on PR commits":

Limitations

The drawback of this method is that you need to be logged in to download the diffs.

pasrom commented 10 months ago

In response to the challenges I faced with secrets management in GitHub Actions (Resource not accessible by integration as seen in this workflow run), I've implemented a secure, two-stage GitHub Actions workflow for KiCad diff generation. This approach avoids the limitations and security concerns associated with using pull_request_target, establishing a more secure procedure for workflows triggered by forks.

To address this, I adopted best practices for security based on guidance from the GitHub Security Lab's article. My solution involves using two separate workflow files: kibot_diff.yml for running the KiBot diff process, and kibot_run_comment.yml for safely posting the results on pull requests.

One drawback of this method is that the changes cannot be tested directly in this pull request. However, to ensure functionality and reliability, I have thoroughly tested these changes in a separate environment, as demonstrated in this test pull request: https://github.com/e-battery-systems/bms-c1-tests/pull/4.

dmohns commented 10 months ago

To address this, I adopted best practices for security based on guidance from the GitHub Security Lab's article. My solution involves using two separate workflow files: kibot_diff.yml for running the KiBot diff process, and kibot_run_comment.yml for safely posting the results on pull requests.

Today I learned 😉 The article you linked is a good read! I didn't know about this, thanks for sharing. Should we add a small comment on top of kibot_run_comment.yml linking the article to explain why this is in a different file?

Other than that, LGTM. Leaving final approval to @martinjaeger again

pasrom commented 10 months ago

Should we add a small comment on top of kibot_run_comment.yml linking the article to explain why this is in a different file?

Absolutely, adding a comment to kibot_run_comment.yml with a link to the article for context sounds good. I'll take care of it by this evening.