👷 Add sticky helm diff PR comment

[ddelange]

ref #62 (comment)

example ddelange#5 (comment)

[ddelange]

should start working once this workflow lives in twuni:main ref

[ddelange]

tested here ddelange#2

[ddelange]

now with filenames in the diff, and collapsed the markdown ddelange#5

[canterberry]

Thanks, this is great! It will save time on PR review, for sure.

I have a couple of concerns, which may or may not require adjustments. Just talking points to discuss before merge:

1. marocchino/sticky-pull-request-comment is a privately maintained GitHub action, so I'm concerned about introducing a dependency on it. It creates a single point of failure and a heightened security risk compared to, say, a more popular and commercially-backed/community-led action. Not a blocking issue, but just wanted to bring it up.
2. If the action is failing on this PR, what evidence supports that it would succeed on subsequent PRs? Are any repo configuration changes necessary in order to support the action-initiated PR comment? I can see it passing on your fork, but I don't yet understand why it passes there but fails here. I suspect it's because the branch the PR is based on is local to the target branch (vs any PRs opened against this repo which are coming from forks).

While (1) is not something I'm that concerned about, I'd like to understand (2) better to avoid potentially breaking mergability of PRs in this repo, although the risk is low, because if that were to happen, it would be trivial to fix by reverting this PR.

[ddelange]

Very valid concerns!

1. could be remedied by pointing to a commit hash instead of a major version
2. is probably a valid issue ref 'Send PR comment' step not working for people creating the PR that don't have permission to the repository keptn-contrib/artifacthub#89. I think you might be right that in the current state it won't work for any new PRs coming from forks, even when this code would live on twuni:main. I encountered the 'not on master' behaviour in a different setting, and must have gotten them confused. I think it had to do with the release trigger, not pull_request. Fix would be to run on pull_request_target trigger, barring all permissions to the target repo except for the pull request itself:

pull_request_target: "This event allows your workflow to do things like label or comment on pull requests from forks."

[canterberry]

Thanks for (1) updating to the commit hash, and for the link on (2). This comment points out three potential solutions, and unfortunately, neither of them are viable. Granting the Action read/write repository permissions with access to secrets even when running on a fork is a non-starter:

TL;DR: Combining pull_request_target workflow trigger with an explicit checkout of an untrusted PR is a dangerous practice that may lead to repository compromise.

The value provided by this PR, however, is great enough that I'd still love to see it land in some form. Is there another way the diff can be presented or attached? I'm not familiar with GitHub Actions (my experience is mostly CircleCI and GitLab) -- is there something like Artifacts or Reports that can be attached to the action's result?

---

Edit: I'm reading the "Preventing pwn requests" post more thoroughly, where the example given is automating a PR comment. If this can be done safely, I do think a PR comment is more discoverable. It's just not clear to me (yet?) that it's possible to do so in a safe enough way.

[canterberry]

👍 Although the newly introduced action is currently failing, it seems probable that is because it's trying to do something the target hasn't authorized. Post-merge, the target will have authorized the action, so in theory will start working. If not, it's trivial to revert.

[canterberry]

Released in v2.2.1.