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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Combine create and update in a single configuration? #9

Closed
robdodson opened this issue Mar 7, 2020 · 8 comments 路 Fixed by #16
Closed

Combine create and update in a single configuration? #9

robdodson opened this issue Mar 7, 2020 · 8 comments 路 Fixed by #16

Comments

@robdodson
Copy link
Contributor

Hello 馃憢

I was wondering if you would consider a PR which slightly changes the behavior of the Action to allow someone to create or update in a single configuration? My use case is I wanted to have a comment at the top of my PR that shows lint status. If it's the first the linter has run, it will create this comment. On subsequent runs, it will update this comment using replace.

Currently the Action will always create a new comment if an issue number is passed. Here's the relevant snippet of code:

if (inputs.issueNumber) {
  ...
} else if (inputs.commentId) {
  ...
}

However, if you swap the order of the condition, then you can have it first check for a comment-id to update, and if one isn't passed it will check for an issue number to create a new comment:

if (inputs.commentId) {
  ...
} else if (inputs.issueNumber) {
  ...
}

That enables a workflow like this:

# This finds the first comment id from the GitHub Actions bot.
# If it hasn't commented on the issue or PR yet then it returns an empty string.
- name: Find comment
  id: find_comment
  uses: ./.github/actions/find-comment
  with:
    token: ${{ secrets.GITHUB_TOKEN }}

- name: Create or update comment
  # I forked a version of this repo to implement the above mentioned conditional
  uses: robdodson/create-or-update-comment@master
  with:
    token: ${{ secrets.GITHUB_TOKEN }}
    issue-number: ${{ github.event.number }}
    # If the comment-id is blank, this will create a new comment.
    # Otherwise, it will update the comment.
    comment-id: ${{ steps.find_comment.outputs.comment-id }}
    body: |
      Create or update this comment.
    edit-mode: replace
  if: success()

I realize you could also achieve this by writing two steps and using if to check if it should create or update the comment, but that felt a bit noisy to me. If you'd be open to this change then I could send over a PR. I also understand if you'd prefer to keep the Action as is because it matches your use case :)

@peter-evans
Copy link
Owner

Hi @robdodson

I think allowing this additional use case would be a good improvement to the action. It doesn't look like it would be a breaking change for the way the action is currently being used. I would very much welcome a PR for this change.

@peter-evans
Copy link
Owner

Hi @robdodson. Let me know if you are too busy and I can apply this update myself.

@robdodson
Copy link
Contributor Author

Oh sorry, I was trying to track down the comment today to work on this. If you are already working on the repo and feel like doing it that would be great 馃槉

@peter-evans
Copy link
Owner

No worries. I'm not working on the repo, just a reminder. I'll let you raise the PR for it since it was your idea. 馃槃

@peter-evans
Copy link
Owner

Released as @v1.1.0 or follow @v1.
Thanks again!

@m-kuhn
Copy link

m-kuhn commented Mar 31, 2020

Nice suggestion.

Do you already have an example for the find-comment action?

@robdodson
Copy link
Contributor Author

Not yet. Will see about open sourcing it at some point.

@m-kuhn
Copy link

m-kuhn commented Apr 1, 2020

For those looking for an ad hoc solution (or some inputs for how to implement something proper)

https://github.com/opengisch/QField/blob/7bbc041fd467c5e6a41796f20cc120c8f77fd6d9/.github/workflows/clang-format.yml#L18-L57

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

Successfully merging a pull request may close this issue.

3 participants