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

fix: include tags in variants event #4711

Merged
merged 1 commit into from
Sep 14, 2023
Merged

Conversation

nunogois
Copy link
Member

https://linear.app/unleash/issue/2-1396/fix-variants-event-should-include-the-respective-feature-flag-tags

This fixes an issue where the feature-environment-variants-updated event would not post to a tagged Slack channel, since it would not take into consideration the feature flag tags.

@vercel
Copy link

vercel bot commented Sep 14, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Sep 14, 2023 10:06am
unleash-monorepo-frontend ⬜️ Ignored (Inspect) Sep 14, 2023 10:06am

Copy link
Contributor

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

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

I think this solves the problem, but... where should we add this query for tags?

We're adding a side effect on the creation of the event to add something that's unrelated with the event itself. Should we do this query instead in the SlackApp addon when we receive an event with a featureName and tags === undefined?

Copy link
Contributor

@chriswk chriswk left a comment

Choose a reason for hiding this comment

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

Nice catch.

@nunogois nunogois merged commit 31216d1 into main Sep 14, 2023
10 checks passed
@nunogois nunogois deleted the fix-event-variants-include-tags branch September 14, 2023 11:16
@nunogois
Copy link
Member Author

I think this solves the problem, but... where should we add this query for tags?

We're adding a side effect on the creation of the event to add something that's unrelated with the event itself. Should we do this query instead in the SlackApp addon when we receive an event with a featureName and tags === undefined?

Good point, however this seems like our approach for other events as well, so at least it's consistent. There are many examples of this exact flow in feature-toggle-service. Perhaps we should evaluate whether there's a better way of fetching the feature tags, but I think it falls outside of the scope for this fix.

@gastonfournier
Copy link
Contributor

Good point, however this seems like our approach for other events as well, so at least it's consistent. There are many examples of this exact flow in feature-toggle-service. Perhaps we should evaluate whether there's a better way of fetching the feature tags, but I think it falls outside of the scope for this fix.

Ok, standardization is a good reasoning to continue with this idea, I'm just thinking that by following my proposal, we can remove all this duplicated code, and we may even catch other events that are in the same situation. After all, not all listeners of the event need to know about the tags, we're adding this information just because of the way we built Slack Addon, therefore we're coupling our code to one particular solution.

But I agree, we can consider it out of the scope, especially if this involves changing other unrelated parts of the codebase.

nunogois added a commit that referenced this pull request Sep 15, 2023
https://linear.app/unleash/issue/2-1396/fix-variants-event-should-include-the-respective-feature-flag-tags

This fixes an issue where the `feature-environment-variants-updated`
event would not post to a tagged Slack channel, since it would not take
into consideration the feature flag tags.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants