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

Setting emoji-reaction to an Empty String does not Disable Comment Reactions #3457

Open
X-Guardian opened this issue May 27, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@X-Guardian
Copy link
Contributor

Community Note

  • Please vote on this issue by adding a 馃憤 reaction to the original issue to help the community and maintainers prioritize this request. Searching for pre-existing feature requests helps us consolidate datapoints for identical requirements into a single place, thank you!
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment.

Overview of the Issue

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.

Reproduction Steps

config.yaml

emoji-reaction: ""

GitHub Screenshot

image

Environment details

Environment details
Atlantis version: 0.23.5
Deployment method: local
If not running the latest Atlantis version have you tried to reproduce this issue on the latest version: n/a
Atlantis flags: none

Additional Context

This is due to the default emoji reaction logic overriding the empty string here:

atlantis/cmd/server.go

Lines 801 to 803 in 998eb63

if c.EmojiReaction == "" {
c.EmojiReaction = DefaultEmojiReaction
}

Suggestion is to either add a new property to suppress the emoji reactions, e.g. disable-emoji-reactions or to use a 'magic value' for emoji-reaction to disable the reactions instead, e.g. **Disabled**.

@X-Guardian X-Guardian added the bug Something isn't working label May 27, 2023
@nitrocode
Copy link
Member

cc @marcusramberg

@bschaatsbergen
Copy link
Contributor

Working on this... :)

@jamengual
Copy link
Contributor

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

@bschaatsbergen
Copy link
Contributor

bschaatsbergen commented Oct 21, 2023

it should be disabled by default like all the other flags

@jamengual, as in by default an emoji should never be reacted on a plan or apply comment in the first place? That would change the current default behaviour though, but it's not impactful if we want to make this change.

@jamengual
Copy link
Contributor

yes, when we add features they are all behind a flag and disabled by default, so in this case, no emoji should be posted and if enabled an emoji should be configured.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants