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: do not react with emoji if "EmojiReaction" flag is empty #4378

Merged

Conversation

CaioAugustoo
Copy link
Contributor

@CaioAugustoo CaioAugustoo commented Mar 21, 2024

what

  • do not react comments with emoji in case "EmojiReaction" flag is empty

why

According to #3360: "setting the emoji-reaction property to an empty string should disable comment reactions. This is not working, and the default eyes emoji continues to be used."

Also, as @jamengual mentioned: "It sounds weird to me that we will add another config flag to disable the emojis, I think this logic needs to be fixed. it should be disabled by default like all the other flags"

This PR makes "EmojiReaction" flag default value to an empty string. In case "EmojiReaction" flag is empty no comment will be reacted.

tests

  • I have tested my changes by running local tests with make test
  • I have tested my changes by running local tests with make build
  • I have tested my changes by running local tests in my own repository

references

@CaioAugustoo CaioAugustoo requested review from a team as code owners March 21, 2024 21:18
@CaioAugustoo CaioAugustoo requested review from lkysow, lukemassa and nitrocode and removed request for a team March 21, 2024 21:18
@github-actions github-actions bot added docs Documentation go Pull requests that update Go code labels Mar 21, 2024
@CaioAugustoo CaioAugustoo changed the title Enable emoji reaction feat: create flag to toggle emoji reactions Mar 21, 2024
@GenPage GenPage removed the request for review from lkysow March 22, 2024 08:42
@jamengual
Copy link
Contributor

Hi, @CaioAugustoo, why do we not make --emoji-reaction=" default to empty and fix the logic if needed instead of adding a flag to disable it?

The whole idea about my comment is that the flag should always be disabled by default.

@CaioAugustoo CaioAugustoo changed the title feat: create flag to toggle emoji reactions feat: do not react with emoji if "EmojiReaction" flag is empty Mar 24, 2024
@CaioAugustoo
Copy link
Contributor Author

CaioAugustoo commented Mar 24, 2024

Hi, @CaioAugustoo, why do we not make --emoji-reaction=" default to empty and fix the logic if needed instead of adding a flag to disable it?
The whole idea about my comment is that the flag should always be disabled by default.

@jamengual, Done! I made some changes. Let me know if it makes sense for you!

jamengual
jamengual previously approved these changes Apr 2, 2024
Copy link
Contributor

@jamengual jamengual left a comment

Choose a reason for hiding this comment

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

Thanks for the work @CaioAugustoo

@jamengual jamengual added this to the v0.28.0 milestone Apr 2, 2024
@jamengual jamengual added refactoring Code refactoring that doesn't add additional functionality and removed docs Documentation labels Apr 2, 2024
@CaioAugustoo
Copy link
Contributor Author

Thank you @jamengual!

@chenrui333 chenrui333 requested review from a team and GenPage and removed request for a team May 21, 2024 21:43
@chenrui333
Copy link
Member

not quite sure why e2e tests run on the merge commit

@chenrui333 chenrui333 merged commit 388bbfd into runatlantis:main May 22, 2024
27 of 28 checks passed
@chenrui333
Copy link
Member

Thanks @CaioAugustoo for your first contribution to atlantis! 💯

ragne pushed a commit to ragne/atlantis that referenced this pull request May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation go Pull requests that update Go code ready to merge refactoring Code refactoring that doesn't add additional functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants