Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add plan results to PR as a comment #35

Closed
xt0rted opened this issue Jan 8, 2021 · 9 comments · Fixed by #36
Closed

Add plan results to PR as a comment #35

xt0rted opened this issue Jan 8, 2021 · 9 comments · Fixed by #36
Assignees
Labels
enhancement New feature or request

Comments

@xt0rted
Copy link
Contributor

xt0rted commented Jan 8, 2021

Is your enhancement request related to a problem? Please describe.

Right now you have to look through the log output to see what the proposed changes are, it'd be a lot nicer if this could be added to the PR as a comment for easier review.

Describe the solution you'd like

I'd like to see a plan logger that comments on the PR, if being run from one, with a table of the proposed changes. There's a version of this in the octoDNS repo which can be found in octodns/octodns#156 (comment) along with examples of what the comment looks like.

Describe alternatives you've considered

Creating my own action/scripts which I'd rather not do since I've never worked with python.

Additional context

@xt0rted xt0rted added the enhancement New feature or request label Jan 8, 2021
@solvaholic solvaholic self-assigned this Jan 9, 2021
@solvaholic
Copy link
Owner

I like this idea, thank you @xt0rted !

To be sure I'm thinking clearly about what you're after.. Would it meet your needs to add an octodns-sync input like add-pr-comment:

        uses: solvaholic/octodns-sync@v2
        with:
          config_path: public.yaml
          add-pr-comment: Yes

And, when that's set, octodns-sync adds the HTML-formatted plan comment to the PR that triggered the action, like in the example you provided?

Adding a comment will be straightforward, with pull_request.issue_url in the triggering payload. I'll figure out how to get the planhtml output and put together something we can test.

@xt0rted
Copy link
Contributor Author

xt0rted commented Jan 10, 2021

Yea, I figured making this opt-in would be the best way to do it. It's been awhile since I played with octodns but from what I recall the code in that issue worked fine for me back when it was first posted. At the time I was running it locally, I never got far enough to deploy it so hopefully this is even easier now with actions.

The other settings that might be useful are one for the api token and the name & email of the account leaving the comment.

@solvaholic
Copy link
Owner

Yea, I figured making this opt-in would be the best way to do it.

10-4, thank you for confirming.

The other settings that might be useful are one for the api token and the name & email of the account leaving the comment.

👍, adding the PR comment may need a token. Would you use the name and email for attributing the comment? or something else?

I'll figure out how to get the planhtml output...

After adding this to /config/public.yaml:

manager:
  plan_outputs:
    html:
      class: octodns.provider.plan.PlanHtml

This command:

octodns-sync --config-file="/config/public.yaml" 2>/dev/null

Produced this output:

example.com.

route53

Operation Name Type TTL Value Source
Create llama CNAME 3600 nnn01.example.com. config
Summary: Creates=1, Updates=0, Deletes=0, Existing Records=17

@solvaholic
Copy link
Owner

I'm thinking:

  • The plan is what @thattommyhall wanted, in add result output #11, not necessarily the rest of the octodns-sync output
  • Write the log to one file and the plan to another
  • If add-pr-comment is enabled, post the contents of the output file as a comment on the triggering pull request
  • This'll fit naturally into entrypoint.sh

This shell script:

#!/bin/bash
_needle='<!--- octodns/plan --->'
_header="## octoDNS Plan for {sha}"
_footer="Automatically generated by [solvaholic/octodns-sync](https://github.com/solvaholic/octodns-sync)"

_config_path="/config/public.yaml"
_planfile="/octodns-sync.plan"
_logfile="/octodns-sync.log"

echo "${_needle}" > "${_planfile}"
echo " " >> "${_planfile}"
echo "${_header}" >> "${_planfile}"
echo " " >> "${_planfile}"

script "${_logfile}" -e -c "octodns-sync --config-file=\"${_config_path}\" >>\"${_planfile}\""

echo " " >> "${_planfile}"
echo "${_footer}" >> "${_planfile}"

Produces the output similar to the example:


octoDNS Plan for {sha}

winans.net.

route53

Operation Name Type TTL Value Source
Create llama CNAME 3600 ams01.winans.net. config
Summary: Creates=1, Updates=0, Deletes=0, Existing Records=17

Automatically generated by solvaholic/octodns-sync

@xt0rted
Copy link
Contributor Author

xt0rted commented Jan 10, 2021

Would you use the name and email for attributing the comment? or something else?

After looking this over again the name & email options aren't needed since there's no commit activity. The reason I mentioned it is because I try to keep automated activity associated with the github-actions[bot]/github-actions[bot]@users.noreply.github.com user so it doesn't get confused with explicit user activity. Since this is an issue comment the user it's associated with is based on the token used which is the bot account for GITHUB_TOKEN.

I like the idea of saving the plan output. Can this be saved as json or yaml? That could make it even more useful if you wanted to work with the data in a later step.

For now I just want a comment on the PR, but down the road I might want to post to slack the changes that were just applied. I don't want to ask for features I may never use though.

solvaholic added a commit that referenced this issue Jan 11, 2021
@solvaholic
Copy link
Owner

After looking this over again the name & email options aren't needed since there's no commit activity.

Cool, thank you for explaining @xt0rted

I like the idea of saving the plan output. Can this be saved as json or yaml? That could make it even more useful if you wanted to work with the data in a later step.

I think that functionality could definitely be added. The plan output is produced by octodns's plan.py, which currently has Plans for Logger, Markdown, and Html. As long as the sync command puts the output you're looking for on stdout, it'll be saved as the plan output.

For now I just want a comment on the PR, but down the road I might want to post to slack the changes that were just applied. I don't want to ask for features I may never use though.

I appreciate your requests, and your consideration 🙇

As of d42b3f1 the issue35 branch is functional enough to test. Most of the details to test it are already in the readme. Here's the gist:

  • Set plan_outputs in your octodns config, for example public.yaml:

    manager:
      plan_outputs:
        html:
          class: octodns.provider.plan.PlanHtml
  • Configure your workflow like:

    on:
      pull_request:
    jobs:
      test:
        runs-on: ubuntu-latest
        steps:
          - uses: actions/checkout@v2
          - uses: solvaholic/octodns-sync@issue35
            with:
              config_path: public.yaml
              add_pr_comment: 'Yes'
              pr_comment_token: '${{ github.token }}'

If you have a chance to try it out, please let me know what you think.

@solvaholic
Copy link
Owner

@xt0rted this feature is available and documented in main now, so uses: solvaholic/octodns-sync@main should get it.

Please let me know what questions or feedback you have. 🙇

@xt0rted
Copy link
Contributor Author

xt0rted commented Jan 23, 2021

I'm just now testing this update and have a few issues I'm running into.

The first thing I noticed was if there are errors running OctoDNS a comment is still left on the pr. I don't think this is really an issue, it's just not what I expected since OctoDNS didn't successfully run. I figured this step would throw an error and the workflow would end here. My initial run had the wrong api credentials which resulted in the following log output:

Traceback (most recent call last):
  File "/usr/local/bin/octodns-sync", line 8, in <module>
    sys.exit(main())
  File "/usr/local/lib/python3.7/site-packages/octodns/cmds/sync.py", line 37, in main
    manager = Manager(args.config_file)
  File "/usr/local/lib/python3.7/site-packages/octodns/manager.py", line 118, in __init__
    self.providers[provider_name] = _class(provider_name, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/octodns/provider/rackspace.py", line 64, in __init__
    auth_token, dns_endpoint = self._get_auth_token(username, api_key)
  File "/usr/local/lib/python3.7/site-packages/octodns/provider/rackspace.py", line 85, in _get_auth_token
    x['name'] == 'cloudDNS'][0]['endpoints'][0]['publicURL']
IndexError: list index out of range

And this is the comment that was left:

github com_paramax_dns_pull_2

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.

The third is I keep getting an error saying my zone file isn't found. This is resolved, my config paths were wrong as I expected.

@solvaholic
Copy link
Owner

I'm just now testing this update and have a few issues I'm running into.

Fantastic, thank you for letting me know @xt0rted 🙇

I'll set up new issues for these now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants