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

feat: Github reaction emojis on PR comments #2706

Merged
merged 4 commits into from
Apr 25, 2023

Conversation

marcusramberg
Copy link
Contributor

@marcusramberg marcusramberg commented Nov 21, 2022

what

Adds 👀 reaction to the comment whenever it detects an atlantis command.

I've tested this on a dev instance of Atlantis and it works as expected for my trivial test cases,
but not sure what kind of unit tests you'd like to see for this, so opening it for discussion.
I guess this could also be behind a command line flag, but I'm not sure if that's necessary?

Note that I've stubbed the other VCS providers, as I don't have access to any of them for testing,
I suppose at least bitbucket could support this feature as well tho.

why

  • Emojis (github reactions) on atlantis command comments like atlantis apply can give us assurance that Atlantis has picked up the job

references

body := event.GetComment().GetBody()

if strings.HasPrefix(body, "atlantis ") {
err = e.VCSClient.ReactToComment(baseRepo, *event.Comment.ID, "eyes")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the pr.

Please make the emojis configurable from the server config and overwritable in the repo config

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice feature. The rapid feedback improves the developer experience. I also want to add 🚀 .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to move react logic in here.

logger.Debug("executing command")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krrrr38 I would have preferred that as well, but there you don't have access to the event object anymore so I don't see a way to get to the comment id.

@@ -181,6 +181,13 @@ func (g *GithubClient) CreateComment(repo models.Repo, pullNum int, comment stri
return nil
}

// ReactToComment adds a reaction to a comment.
func (g *GithubClient) ReactToComment(repo models.Repo, commentID int64, reaction string) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests

@nitrocode nitrocode added waiting-on-response Waiting for a response from the user waiting-on-review Waiting for a review from a maintainer needs tests Change requires tests labels Nov 24, 2022
@nitrocode
Copy link
Member

@marcusramberg friendly ping. I'd love to see this in the next release if you have the time 😄 🙏

@marcusramberg
Copy link
Contributor Author

@marcusramberg friendly ping. I'd love to see this in the next release if you have the time 😄 🙏

Got a bit stuck on rewriting the mocks but will try to refocus on completing this PR this week.

@nitrocode nitrocode changed the title Basic implementation of github reactions on PRs feat: Github reaction emojis on PR comments Dec 19, 2022
@nitrocode
Copy link
Member

Got a bit stuck on rewriting the mocks but will try to refocus on completing this PR this week.

You mean regenerating the mocks using pegomock?

@nitrocode nitrocode marked this pull request as draft December 22, 2022 22:40
@marcusramberg
Copy link
Contributor Author

Got a bit stuck on rewriting the mocks but will try to refocus on completing this PR this week.

You mean regenerating the mocks using pegomock?

No, I meant I started a branch to rewrite the mocks using gomock. Regenerating went well but it's a lot of work to rewire the tests to use the gomock apis, I have a WIP branch here main...marcusramberg:atlantis:marcus/new_mock - although it's now already conflicting a lot with latest main :)

@nitrocode
Copy link
Member

I think the gomock rewrite is noble but can be done in a separate PR. The pegomock should be good enough for this PR, no?

@marcusramberg marcusramberg force-pushed the emoji_reactions branch 4 times, most recently from 58a0a25 to 5268f5e Compare December 26, 2022 20:06
@nitrocode nitrocode modified the milestones: 0.22.2, 0.22.3, 0.23.0 Jan 6, 2023
@nitrocode nitrocode modified the milestones: v0.23.0, v0.24.0 Jan 21, 2023
@github-actions
Copy link

This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month.

@github-actions github-actions bot added the Stale label Mar 12, 2023
@marcusramberg
Copy link
Contributor Author

Finally got around to write a test for this, sorry for the delay.

GenPage
GenPage previously approved these changes Apr 20, 2023
Copy link
Member

@GenPage GenPage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for your contribution

@GenPage
Copy link
Member

GenPage commented Apr 21, 2023

Rebased to pull in workflow fixes from #3342

@GenPage GenPage enabled auto-merge (squash) April 21, 2023 14:16
@GenPage
Copy link
Member

GenPage commented Apr 21, 2023

@marcusramberg The linter failures might not be all of your changes, but for the sake can you take a pass and see if any apply? Thanks

@marcusramberg
Copy link
Contributor Author

marcusramberg commented Apr 21, 2023

@marcusramberg The linter failures might not be all of your changes, but for the sake can you take a pass and see if any apply? Thanks
That's a lot of linter errors. Seems like there's only one that applies to this PR:

image

Tbh I'm not sure if I agree with this error since this is a stub function that hopefully should be implemented later for gitlab, but if you feel it should be adressed I can change it to _

The function would look like this then:
image

auto-merge was automatically disabled April 23, 2023 18:58

Head branch was pushed to by a user without write access

@GenPage GenPage enabled auto-merge (squash) April 25, 2023 21:22
Copy link
Member

@GenPage GenPage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this.

@GenPage GenPage merged commit f8b293a into runatlantis:main Apr 25, 2023
nitrocode pushed a commit that referenced this pull request May 5, 2023
* feat: Basic implementation of github reactions on PRs

Adds eyes whenever it detects an `atlantis` command.

* feat: Make the emoji reaction configurable

* tests: Add a mocked test for EmojiReaction being called in github

* ci: Disable revive linter for stubs
mtavaresmedeiros pushed a commit to mtavaresmedeiros/atlantis that referenced this pull request Jul 3, 2023
* feat: Basic implementation of github reactions on PRs

Adds eyes whenever it detects an `atlantis` command.

* feat: Make the emoji reaction configurable

* tests: Add a mocked test for EmojiReaction being called in github

* ci: Disable revive linter for stubs
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
* feat: Basic implementation of github reactions on PRs

Adds eyes whenever it detects an `atlantis` command.

* feat: Make the emoji reaction configurable

* tests: Add a mocked test for EmojiReaction being called in github

* ci: Disable revive linter for stubs
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
* feat: Basic implementation of github reactions on PRs

Adds eyes whenever it detects an `atlantis` command.

* feat: Make the emoji reaction configurable

* tests: Add a mocked test for EmojiReaction being called in github

* ci: Disable revive linter for stubs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code needs tests Change requires tests provider/azuredevops provider/bitbucket provider/github provider/gitlab waiting-on-response Waiting for a response from the user waiting-on-review Waiting for a review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants