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

Show GitHub comments in PR timeline for push events #387

Closed
micnncim opened this issue Aug 5, 2022 · 8 comments
Closed

Show GitHub comments in PR timeline for push events #387

micnncim opened this issue Aug 5, 2022 · 8 comments
Labels
enhancement New feature or request
Milestone

Comments

@micnncim
Copy link
Contributor

micnncim commented Aug 5, 2022

GitHub no longer shows commit comments in the pull request timeline 1.

It'd be worth considering an option to show comments as PR comments, not commit comments, even for push events or triggers, since some users would expect the behavior as it was and users need to jump to a commit page to see comments from tfcmt.

Currently, tfcmt creates commit comments if a PR number isn't found 2 so fetching a PR number with a commit SHA before creating a comment would work, if the commit belongs to a PR.

Footnotes

  1. https://github.blog/changelog/2022-08-04-commit-comments-no-longer-appear-in-the-pull-request-timeline/

  2. https://github.com/suzuki-shunsuke/tfcmt/blob/d4239c42b059961c4379ba731726312b80290667/pkg/notifier/github/comment.go#L22-L41

@micnncim micnncim changed the title Show comments in PR timeline for push events Show GitHub comments in PR timeline for push events Aug 5, 2022
@suzuki-shunsuke suzuki-shunsuke added the enhancement New feature or request label Aug 5, 2022
@suzuki-shunsuke
Copy link
Owner

Thank you for your proposal.
It looks good.

@suzuki-shunsuke
Copy link
Owner

suzuki-shunsuke commented Aug 5, 2022

Let's consider the specification.

Default behavior

  1. ⭕ Change the default behavior
  2. Keep the default behavior

Strictly this may be a breaking change, but I think we don't have to treat this as a breaking change.

Configuration

  1. ⭕ Don't allow to change the behavior
  2. Allow to change the behavior

We can support configuration later.

How to configure

  1. Configuration file
  2. Environment variable
  3. Command line option

How to handle when multiple pull requests are associated

  1. Choose the first pull request in the array
  2. ⭕ Choose the latest updated pr
  3. Don't choose pr and send a comment to a commit

How to handle when it fails to get associated pull requests

  1. ⭕ Send a comment to a commit and exits with zero
  2. Send a comment to a commit and exits with non zero
  3. Don't send a comment and exit with non zero

@micnncim
Copy link
Contributor Author

micnncim commented Aug 6, 2022

@suzuki-shunsuke Thanks for your consideration. Overall, I agree with all your points.

Default behavior

  1. ⭕ Change the default behavior
  2. Keep the default behavior

Strictly this may be a breaking change, but I think we don't have to treat this as a breaking change.

Since the GitHub change occurred just 2 days ago and the existing users have seen comments in the PR timeline for push events so far, most of them would expect tfcmt posts comments as it was.

Configuration

  1. ⭕ Don't allow to change the behavior
  2. Allow to change the behavior

We can support configuration later.

Opt-out will be able supported later but it should be noted that comments will be being shown as the PR comments for new users, who might be confused.

How to handle when it fails to get associated pull requests

  1. ⭕ Send a comment to a commit and exits with zero
  2. Send a comment to a commit and exits with non zero
  3. Don't send a comment and exit with non zero

Yeah, posting commit comments should be fallback because it's quite common that commits aren't associated with a PR (e.g. merge commits, commits before creating a PR).

Are you willing to work on implementation? If you're busy, I'm happy to get it work.

@suzuki-shunsuke
Copy link
Owner

Thank you for your feedback.

Are you willing to work on implementation? If you're busy, I'm happy to get it work.

I really appreciate if you work on this issue.

@suzuki-shunsuke
Copy link
Owner

@suzuki-shunsuke
Copy link
Owner

suzuki-shunsuke commented Aug 7, 2022

Test

$ tfcmt -v
tfcmt version 3.4.0-1 (48333cac413f4500b2631eebb9703a9edc5e0a9d)

$ terraform -v
Terraform v1.2.6
on darwin_arm64
+ provider registry.terraform.io/hashicorp/null v3.1.1
export GITHUB_TOKEN=xxx
export CIRCLECI=true
export CIRCLE_PROJECT_USERNAME=suzuki-shunsuke
export CIRCLE_PROJECT_REPONAME=test-github-action
export CIRCLE_SHA1=092cda867dea7b7a229bc2dcff64aaba8fb26f8d

suzuki-shunsuke/test-github-action@092cda8

$ tfcmt plan -- terraform plan
null_resource.foo: Refreshing state... [id=8966706083595703589]
null_resource.bar: Refreshing state... [id=7780302426904623778]

No changes. Your infrastructure matches the configuration.

Terraform has compared your real infrastructure against your configuration
and found no differences, so no changes are needed.

suzuki-shunsuke/test-github-action#104 (comment)

👍 It works well.


In case of tfcmt v3.3.0, a comment is created to a commit.

suzuki-shunsuke/test-github-action@092cda8#commitcomment-80523712

@suzuki-shunsuke
Copy link
Owner

Test Apply

suzuki-shunsuke/test-github-action@34f92a4

export GITHUB_TOKEN=xxx
export CIRCLECI=true
export CIRCLE_PROJECT_USERNAME=suzuki-shunsuke
export CIRCLE_PROJECT_REPONAME=test-github-action
export CIRCLE_SHA1=34f92a42f1473774f452d07c574a01069aadbff4
$ tfcmt apply -- terraform apply -no-color -auto-approve
null_resource.foo: Refreshing state... [id=8966706083595703589]
null_resource.bar: Refreshing state... [id=7780302426904623778]

No changes. Your infrastructure matches the configuration.

Terraform has compared your real infrastructure against your configuration
and found no differences, so no changes are needed.

Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

suzuki-shunsuke/test-github-action#45 (comment)

👍 It works well.

@suzuki-shunsuke
Copy link
Owner

Released https://github.com/suzuki-shunsuke/tfcmt/releases/tag/v3.4.0

Thank you for your contribution!

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

No branches or pull requests

2 participants