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

chore: add enterprise event #5056

Merged
merged 2 commits into from
Oct 16, 2023
Merged

chore: add enterprise event #5056

merged 2 commits into from
Oct 16, 2023

Conversation

gastonfournier
Copy link
Contributor

This just adds a new event type

@vercel
Copy link

vercel bot commented Oct 16, 2023

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

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Visit Preview Oct 16, 2023 1:33pm
unleash-monorepo-frontend ⬜️ Ignored (Inspect) Visit Preview Oct 16, 2023 1:33pm

Copy link
Member

@nunogois nunogois left a comment

Choose a reason for hiding this comment

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

LGTM!
I'll leave it up to you whether you consider it's worth it adding this to the event formatter and the list of available events in the Slack App definition.

@gastonfournier
Copy link
Contributor Author

LGTM! I'll leave it up to you whether you consider it's worth it adding this to the event formatter and the list of available events in the Slack App definition.

I think this is a different thread, but shouldn't SlackApp have all events? I feel it's difficult to keep track of every place we need to add the new event to... I rather have slackApp filter out some events but by default have all event types available. WDYT?

@nunogois
Copy link
Member

nunogois commented Oct 16, 2023

LGTM! I'll leave it up to you whether you consider it's worth it adding this to the event formatter and the list of available events in the Slack App definition.

I think this is a different thread, but shouldn't SlackApp have all events? I feel it's difficult to keep track of every place we need to add the new event to... I rather have slackApp filter out some events but by default have all event types available. WDYT?

I agree. In fact, I think all integrations should have all events available to be configured. However we need to take into account that some events may not have a message template in the event formatter (even though there's a fallback) and that some events are deprecated (these could be filtered out manually). Maybe something to try out in a different PR.

@gastonfournier gastonfournier merged commit 675ec2e into main Oct 16, 2023
6 checks passed
@gastonfournier gastonfournier deleted the add-enterprise-event branch October 16, 2023 13:37
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.

2 participants