Improve comment formatting

[travislikestocode]

Description

On my fork I added some flair to the PR comments to make them easier to grok at a glance. I'm submitting the changes as a PR in hopes that @solvaholic and others like them.

Changes:

• Add emoji to the beginning of the header. if $DOIT is '--doit', otherwise: 🧪
• Add 'deploy' to the header if $DOIT is '--doit'. otherwise, add 'dry run'
• Adds input add_pr_comment_footer which is enabled by default, but will hide the footer if anything but 'Yes' is supplied. AFAICT, Github actions don't support boolean inputs. In hindsight, this is probably overkill but I prefer not to have the footer. 🤷
• Add support for pull_request_target

Motivation and Context

Adding support for pull_request_target is helpful for people using the event trigger. The formatting changes are just a quality of life thing that make the comments more useful in my opinion.

How Has This Been Tested?

Created test workflows for pull_request, and pull_request_target. Created a PR, and the actions runs were successful: pull_request, pull_request_target

Example Comment:

octoDNS deployment for 85b0b89

example.com.

etchosts

Operation	Name	Type	TTL	Value	Source
Create	Zone<example.com.>
Create		A	60	1.2.3.5; 1.2.3.6	config

Summary: Creates=1, Updates=0, Deletes=0, Existing Records=0

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• I have read the CONTRIBUTING document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• All new and existing tests passed.

[solvaholic]

Thank you @travislikestocode!

Add support for pull_request_target

I remember the pull_request_target event coming up in #41 (comment). On public repositories I believe triggering workflows on pull_request_target introduces a lot of risk. On private or internal repositories, I think that risk is much smaller.

If we're going to add this I'd like to also add documentation about how to reduce risk while using the pull_request_target event. The documentation can go up separately, doesn't need to complicate this pull request.

Adds input add_pr_comment_footer which is enabled by default...

How would it be to expose the whole header and footer as inputs?

Like, if there were pr_comment_header and pr_comment_footer inputs, so you could specify the markdown in the workflow?

[travislikestocode]

I remember the pull_request_target event coming up in #41 (comment). On public repositories I believe triggering workflows on pull_request_target introduces a lot of risk. On private or internal repositories, I think that risk is much smaller.

Ah yes because it'll run a workflow from the base ref, but it could checkout and run code from the head of the pull request, right?

If we're going to add this I'd like to also add documentation about how to reduce risk while using the pull_request_target event. The documentation can go up separately, doesn't need to complicate this pull request.

Would it be adequate to link to the official warning?

How would it be to expose the whole header and footer as inputs? Like, if there were pr_comment_header and pr_comment_footer inputs, so you could specify the markdown in the workflow?

This makes so much more sense. I'll do that instead.

[solvaholic]

How would it be to expose the whole header and footer as inputs?

I see it now: The flexibility I was thinking of would be one of the benefits of using the 3rd party commenter:

https://github.com/marketplace/actions/create-or-update-comment

@travislikestocode would it meet your needs to use ☝️ that Action to add the pull request comments?

We currently have example implementations in #41 (comment) and in the solvahol/dns repo.

I'm open to extending this Action's built-in pull request updater, and I'm wary of reinventing wheels.

[solvaholic]

Add support for pull_request_target

I'm getting myself hung up on "why are these changes coming from forks?", but I think that's not really relevant. I raised #70 to frame it as an issue with comment.sh and explore how to solve that.

Ah yes because it'll run a workflow from the base ref, but it could checkout and run code from the head of the pull request, right?

Right. Actions triggered by the pull_request_target event run with read/write permission, so more care is required to protect authentication tokens and prevent unauthorized changes.

[travislikestocode]

@travislikestocode would it meet your needs to use ☝️ that Action to add the pull request comments?

Sure, that works.

I'm wary of reinventing wheels.

Makes sense. Keep this module concerned with octodns deploys and let the comment module handle comments. And it's possible to include the plan now that we added the output in #66 like you suggested. Good call.

I'm getting myself hung up on "why are these changes coming from forks?", but I think that's not really relevant. I raised #70 to frame it as an issue with comment.sh and explore how to solve that.

For the record, we have developers submitting draft DNS changes from their forks of the octodns repo, which system engineering reviews and deploys.

Right. Actions triggered by the pull_request_target event run with read/write permission, so more care is required to protect authentication tokens and prevent unauthorized changes.

I have a step in my workflow that verifies a sha256 hash for all of the executable scripts in the repo. Probably overkill for my private instance of github enterprise that's restricted to employees but thought I'd mention it in case it helps someone in the future.

...

Closing since we can get the functionality we need without complicating comment.sh

[solvaholic]

I dig it, thank you @travislikestocode 🙇

For the record, we have developers submitting draft DNS changes from their forks of the octodns repo, which system engineering reviews and deploys.

10-4, thank you for explaining. My experience leaves a huge blind spot for me: I and my teammates have write permission to all the repositories I collaborate on at work, and this project is my first practice working with contributors who use forks. I'll work to ask more questions up-front about how folks use, or would like to use, this Action.

I have a step in my workflow that verifies a sha256 hash for all of the executable scripts in the repo. Probably overkill for my private instance of github enterprise that's restricted to employees but thought I'd mention it in case it helps someone in the future.

Maybe overkill, but I think it's a difficult risk to quantify. Thank you for sharing.

In case you'd like to try it out, I removed the event type check in 1f4d942 for #70. That's available by its SHA and at solvaholic/octodns-sync@issue70, for testing in workflows.