add_pr_comment adds new comment for each run

[solvaholic]

Description

From #35 (comment) :

The second thing is each time the action runs it adds a new comment to the PR which can get very noisy. The logger in octodns/octodns#156 (comment) checks the PR comments for one with <!-- octodns/plan --> and updates it if found or creates a new comment if it wasn't.

Expected Behavior

Update the existing comment, rather than create a new one

Actual Behavior

Each time the action runs it adds a new comment to the PR

Possible Fix

The logger in octodns/octodns#156 (comment) checks the PR comments for one with <!-- octodns/plan --> and updates it if found or creates a new comment if it wasn't.

Steps to Reproduce

1. Enable and configure this action
2. Enable and configure add_pr_comment
3. Run octodns-sync to test changes
4. Push more changes
5. Run octodns-sync to test changes

Context

Your Environment

• Version used: main
• Link to your project:

[solvaholic]

@xt0rted will it meet your needs if this octodns-sync action produces markdown or HTML plan output and your workflow uses another action to add the pull request comment?

In #11 @thattommyhall used https://github.com/marketplace/actions/create-or-update-comment and I expect others will work as well.

I'd prefer the flexibility that approach gives you, and the complexity it'd remove from this project. I'll look for an available comment action that performs well and is simple to implement.

If you - or anyone - see an issue with heading that way, please let me know 🙇

[solvaholic]

/cc @patcon fyi per g0v-network/domains#2

[xt0rted]

@solvaholic yea that should be fine. peter-evans/find-comment should handle getting the existing comment for me and then I can pass the id to another step to handle creating/updating as needed.

[patcon]

Thanks for ping! Sounds like it could work :)

Honestly, I recently came to conclusion that this feature (plan comments on PRs) wasn't actually usable for us, since I'll be expecting vast majority of PRs to come from forks. My impression was that forks couldn't ever use a ephemeral token to comment.

But I'm seeing that maybe this announcement of pull_request_target and workflow_run events perhaps support my use-case: https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/ (Any advice is appreciated on this hypothesis.)

Seems these combinations of 2-3 github actions and arcane features might require some good examples in the docs, and I'm happy to contrib that when I sort it out :)

[solvaholic]

Thanks for confirming, y'all!

I hadn't thought of submitting octodns config changes via forks. I'm glad this came up.

My impression was that forks couldn't ever use a ephemeral token to comment.

Indeed, with the pull_request trigger the {{ github.token }} was not allowed to post the PR comment when the PR head was a fork:

solvaholic/scaling-succotash#3 (comment)

Testing with pull_request_target without risking @main will take a few extra steps because I'd hardcoded the event name check in entrypoint.sh:

octodns-sync/entrypoint.sh

Lines 49 to 57 in b170f24

	if [ "${GITHUB_EVENT_NAME}" = "pull_request" ]; then
	if [ -z "${PR_COMMENT_TOKEN}" ]; then
	echo "FAIL: \$PR_COMMENT_TOKEN is not set."
	exit 1
	fi
	else
	echo "SKIP: \$GITHUB_EVENT_NAME is not 'pull_request'."
	exit 0
	fi

Mentioning this now because I think I'll forget later... The pull_request_target doc includes this warning:

The pull_request_target event is granted a read/write repository token and can access secrets, even when it is triggered from a fork. Although the workflow runs in the context of the base of the pull request, you should make sure that you do not check out, build, or run untrusted code from the pull request with this event. Additionally, any caches share the same scope as the base branch, and to help prevent cache poisoning, you should not save the cache if there is a possibility that the cache contents were altered.

To address this issue I think:

• Prove a method to use the create-or-update-comment action to save octodns-sync plan output to pull request comment
• (Document using @peter-evans' create-or-update-comment with this Action #65) Document example workflow for using the create-or-update-comment action to add octodns-sync plan output to pull request comment

[xt0rted]

I'm setting up 2 instances of this action with PR comments so I can share how to do that once I have everything working.

[solvaholic]

@xt0rted @patcon in case it'll help, I just now got a demo success in solvaholic/scaling-succotash#4

The workflow file that made and updated that comment is:

https://github.com/solvaholic/scaling-succotash/blob/74f2769d677b312e64f6c41e4d61ae14826b0944/.github/workflows/octodns-sync.yml

[solvaholic]

Turns out I'd made that more complicated than it needs to be. This worked fine to both create and update the comment:

      - name: Find Comment
        uses: peter-evans/find-comment@v1
        id: fc
        with:
          issue-number: ${{ github.event.pull_request.number }}
          comment-author: 'github-actions[bot]'
          body-includes: Automatically generated by octodns-sync
      - name: Add or update PR comment
        uses: peter-evans/create-or-update-comment@v1
        with:
          issue-number: ${{ github.event.pull_request.number }}
          comment-id: ${{ steps.fc.outputs.comment-id }}
          body: |
            ## OctoDNS Plan for `${{ steps.meta.outputs.sha }}`

            ${{ steps.meta.outputs.plan }}

            Automatically generated by octodns-sync
          edit-mode: replace

[solvaholic]

Derp. To actually check out the PR head ref - when triggering on pull_request_target - it's necessary to specify it in the workflow. For example:

      - uses: actions/checkout@v2
        with:
          ref: ${{ github.event.pull_request.head.sha }}

I'd missed it in my testing yesterday: The comments added from a pull_request_trigger reflected the configuration in main rather than in the pull request.

Taking that step manually, to check out the PR head, helps me see why this is relevant:

The pull_request_target event is granted a read/write repository token and can access secrets, even when it is triggered from a fork.

If there's a vulnerability in octodns, or in any actions that run, a malicious user could exploit it with read/write access to the root repository.

Running the workflow on pull_request mitigates that risk by running with read-only access. Any approach that checks out the PR head and runs a workflow with a read/write token bypasses that protection.

It should be possible to run the workflow from the fork with a personal access token, tho that may be onerous for contributors.

In any case, I think this is complete now:

Prove a method to use the create-or-update-comment action to save octodns-sync plan output to pull request comment

@xt0rted @patcon what issues or questions do y'all see around using create-or-update-comment to update, rather than create anew, PR comments with the octodns-sync plan?

[xt0rted]

Will this action load the plan and set it as an output or is that something we'll have to do in a separate step?

[patcon]

Feels like quite a nasty amount of surface area. Bugs in any action could lead to escalation, it seems

if i'm understanding correctly, maybe best to use https://github.com/actions/upload-artifact and its complement download-artefact to just bring only config files into the workspace?

Or this exists, but seems quite niche and nonstandard: https://github.com/Bhacaz/checkout-files

[solvaholic]

Will this action load the plan and set it as an output or is that something we'll have to do in a separate step?

The octodns-sync action saves the log and the plan output in files:

_logfile="${GITHUB_WORKSPACE}/octodns-sync.log"
_planfile="${GITHUB_WORKSPACE}/octodns-sync.plan"

So, currently, it's a separate step: Use the Setting the comment body from a file approach to read the plan file into an output. For example:

      - name: A piece of duct tape
        id: meta
        run: |
          _plan="$(cat ${GITHUB_WORKSPACE}/octodns-sync.plan)"
          _plan="${_plan//'%'/'%25'}"
          _plan="${_plan//$'\n'/'%0A'}"
          _plan="${_plan//$'\r'/'%0D'}" 
          echo "::set-output name=plan::${_plan}"

Previously I'd resisted parsing the outputs into environment variables or workflow step outputs. Now that I see how much work that'd save, and I've had some practice setting and using those outputs, I think I'd like to do it.

[solvaholic]

Feels like quite a nasty amount of surface area. Bugs in any action could lead to escalation, it seems

I agree. I think the risk seems easy-enough to manage. But I've been wrong more times than right when I've said "This should be easy!"

if i'm understanding correctly, maybe best to use https://github.com/actions/upload-artifact and its complement download-artefact to just bring only config files into the workspace?

👍 If you could fetch only the relevant config files from the pull request head that should help a lot. You could do that with other actions or script it directly in a workflow step, where you have access to the runner's operating environment:

https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-README.md

Mixing that in with the other bits it could look like this:

on:
  pull_request_target:
jobs:
  test-and-comment:
    runs-on: ubuntu-20.04
    steps:
      - name: Checkout main
        uses: actions/checkout@v2
      - name: Checkout config files from PR
        id: prhead
        run: |
          # Set $_sha to the first 7 char of PR head SHA
          _sha="$(echo "${{ github.event.pull_request.head.sha }}" | cut -c 1-7)"
          # Fetch PR head commit
          git fetch origin refs/pull/${{ github.event.pull_request.number }}/head
          # Checkout *.yaml from PR head commit
          git checkout "$_sha" -- *.yaml
          # Set output 'sha' to $_sha
          echo "::set-output name=sha::${_sha}"
      - name: Test changes to DNS config
        id: octodns-sync
        uses: solvaholic/octodns-sync@main
        with:
          config_path: test-config.yaml
      - name: Get plan output
        id: plan
        run: |
          # Parse plan output into $_plan
          _plan="$(cat ${GITHUB_WORKSPACE}/octodns-sync.plan)"
          _plan="${_plan//'%'/'%25'}"
          _plan="${_plan//$'\n'/'%0A'}"
          _plan="${_plan//$'\r'/'%0D'}" 
          # Set output 'plan' to $_plan
          echo "::set-output name=plan::${_plan}"
      - name: Find Comment
        uses: peter-evans/find-comment@v1
        id: fc
        with:
          issue-number: ${{ github.event.pull_request.number }}
          comment-author: 'github-actions[bot]'
          body-includes: Automatically generated by octodns-sync
      - name: Add or update PR comment
        if: ${{ github.event_name != 'workflow_dispatch' }}
        uses: peter-evans/create-or-update-comment@v1
        with:
          issue-number: ${{ github.event.pull_request.number }}
          comment-id: ${{ steps.fc.outputs.comment-id }}
          body: |
            ## OctoDNS Plan for `${{ steps.prhead.outputs.sha }}`

            ${{ steps.plan.outputs.plan }}

            Automatically generated by octodns-sync
          edit-mode: replace

I tested that ☝️ over in solvaholic/scaling-succotash#9

[solvaholic]

The workflow in #41 (comment) will NOT check new files out of the PR head. You could checkout new config files similarly, would just need to identify them. Modifying the prhead step:

      - name: Checkout config files from PR
        id: prhead
        run: |
          # Set $_sha to the first 7 char of PR head SHA
          _sha="$(echo "${{ github.event.pull_request.head.sha }}" | cut -c 1-7)"
          # Fetch PR head commit
          git fetch origin refs/pull/${{ github.event.pull_request.number }}/head
          # List changed config files in PR head
          _files="$(git diff --name-only HEAD $_sha | grep "\.yaml$")"
          # Checkout config files from PR head commit
          if [ -n "$_files" ]; then git checkout "$_sha" -- $_files; fi
          # Set output 'sha' to $_sha
          echo "::set-output name=sha::${_sha}"

Referencing $_files like this ☝️ behaved differently in zsh than in bash. So maybe worth specifying a default shell:

defaults:
  run:
    shell: bash

[patcon]

Heads up that I got this working in another repo! Thanks so so much @solvaholic :)))

g0v-network/domains#9 (comment)

My learning was that I had to force HTML plan in the workflow by appending to config, and before then, the comment was just empty (as it was parsing the default logger output)

But other than that it went off without a hitch 🎉

[xt0rted]

This past week I reworked my setup with some new workflows and things are working great. I have two workflows set up, one for PRs, and one for deployments which I'm triggering through GitHub's Slack integration. Since keys need to be ordered alphabetically I added yamllint (part of the vms now) to catch those issues before OctoDNS runs.

octodns-validate.yml

name: Validate

on:
  pull_request:
  push:
    branches: [main]

jobs:
  linting:
    runs-on: ubuntu-latest

    steps:
      - name: Checkout repository
        uses: actions/checkout@v2.3.4

      - name: Run yamllint
        run: yamllint .

  validate:
    needs: linting

    if: "${{ github.event_name == 'pull_request' }}"

    runs-on: ubuntu-latest

    steps:
      - name: Checkout repository
        uses: actions/checkout@v2.3.4

      - name: Run octodns-sync
        uses: solvaholic/octodns-sync@a4b4942c255994ea2c6f193dfc472dec8093b219
        with:
          config_path: production.yaml

      - name: Get plan output
        id: meta
        run: |
          # Parse plan output into $_plan
          _plan="$(cat ${GITHUB_WORKSPACE}/octodns-sync.plan)"
          _plan="${_plan//'%'/'%25'}"
          _plan="${_plan//$'\n'/'%0A'}"
          _plan="${_plan//$'\r'/'%0D'}"
          # Set output 'plan' to $_plan
          echo "::set-output name=plan::${_plan}"
          # Set $_sha to the first 7 char of PR head SHA
          _sha="$(echo "${{ github.event.pull_request.head.sha }}" | cut -c 1-7)"
          # Set output 'sha' to $_sha
          echo "::set-output name=sha::${_sha}"

      - name: Find comment
        uses: peter-evans/find-comment@v1.2.0
        id: fc
        with:
          issue-number: ${{ github.event.pull_request.number }}
          comment-author: github-actions[bot]
          body-includes: Automatically generated by octodns-sync

      - name: Add or update PR comment
        uses: peter-evans/create-or-update-comment@v1.4.4
        with:
          issue-number: ${{ github.event.pull_request.number }}
          comment-id: ${{ steps.fc.outputs.comment-id }}
          body: |
            ## OctoDNS Plan for `${{ steps.meta.outputs.sha }}`

            ${{ steps.meta.outputs.plan }}

            Automatically generated by octodns-sync
          edit-mode: replace

octodns-sync.yml

name: Deploy

on:
  deployment

jobs:
  publish:
    runs-on: ubuntu-latest
    steps:
      - name: Checkout repository
        uses: actions/checkout@v2.3.4

      - name: Starting deployment
        uses: bobheadxi/deployments@v0.4.3
        with:
          step: start
          token: ${{ secrets.GITHUB_TOKEN }}
          deployment_id: ${{ github.event.deployment.id }}
          env: ${{ github.event.deployment.environment }}

      - name: Run octodns-sync --doit
        uses: solvaholic/octodns-sync@a4b4942c255994ea2c6f193dfc472dec8093b219
        with:
          config_path: ${{ github.event.deployment.environment }}.yaml
          doit: --doit

      - name: Finished deployment
        uses: bobheadxi/deployments@v0.4.3
        if: always()
        with:
          step: finish
          token: ${{ secrets.GITHUB_TOKEN }}
          env_url: ${{ github.server_url }}/${{ github.repository }}/tree/${{ github.sha }}
          deployment_id: ${{ github.event.deployment.id }}
          status: ${{ job.status }}

[patcon]

Nice! The one downside of printing the plan to PRs, but only sync'ing on "deploy" event instead of "merge", is that if you neglect to do a deploy after each PR merge, then the PR's plan comment will show more than expected by the submitters, as it's checking against live. This makes the plan output a little noisier and less useful. But if you're deploying each PR, then perhaps that's not important to your workflow :)

Since keys need to be ordered alphabetically

I'm not sure octoDNS's rationale for this opinionated default, but fwiw: I just disabled it, as it seems like a needless friction for first-time contributors.
https://github.com/g0v-network/domains/blob/a3cd9ef9fb5795e4b99a832239318f24c5f79d7a/config.yaml#L10

[xt0rted]

I'm doing branch deploys, so once the plan looks good in the PR I'm deploying the PR branch, verifying the changes, then merging the PR to main. If I need to revert my changes I can do another deploy of the main branch and hopefully get back to where I started before the PR was opened.

I've been trying this method of deploys in my newer projects and so far really like how it works since it's easier to patch & deploy something, or just roll back entirely. This is also how GitHub does deploys.