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.

Sign up for GitHub

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

Jump to bottom

feat(specs): add notification settings to tasks [skip-bc] #4297

Merged
merged 9 commits into from
Jan 6, 2025
Merged

feat(specs): add notification settings to tasks [skip-bc] #4297

merged 9 commits into from
Jan 6, 2025

Conversation

millotp
Copy link
Collaborator

@millotp millotp commented Jan 2, 2025 •
edited by jira bot
Loading

🧭 What and Why

🎟 JIRA Ticket: DI-3239

Add notifications to the task API

@millotp millotp self-assigned this Jan 2, 2025
@millotp millotp requested a review from a team as a code owner January 2, 2025 10:25
@millotp millotp requested review from morganleroi and Fluf22 January 2, 2025 10:25
@algolia-bot
Copy link
Collaborator

algolia-bot commented Jan 2, 2025 •
edited
Loading

✔️ Code generated!

Name Link
🪓 Triggered by 9a531d7835a516c55ad8241df7a91366ab86bdd7
🍃 Generated commit 4b6c73bc8a6d0d7e01398c94bc951f4c3d68a1c2
🌲 Generated branch generated/feat/task-email
📊 Benchmark results

Benchmarks performed on the method using a mock server, the results might not reflect the real-world performance.

Language Req/s
go 1646
javascript 1643
php 1511
csharp 1344
java 1136
python 1092
ruby 836
swift 767

Fluf22
Fluf22 reviewed Jan 2, 2025
View reviewed changes
Copy link
Contributor

@Fluf22 Fluf22 left a comment

Choose a reason for hiding this comment

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

Just a typo

@millotp millotp changed the title feat(specs): add notification settings to tasks feat(specs): add notification settings to tasks [skip-bc] Jan 3, 2025
@millotp millotp requested a review from Fluf22 January 3, 2025 08:44
shortcuts
shortcuts reviewed Jan 6, 2025
View reviewed changes
specs/ingestion/common/schemas/task.yml
required:
- email

Policies:
Copy link
Member

Choose a reason for hiding this comment

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

is policies a relevant name in that case? Do we already have other rules in mind than the error thresholds?

Copy link
Member

Choose a reason for hiding this comment

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

also could be great to move the failure threshold there

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes we have other policies in mind, for example the safetyCheck as you talked about on slack, to prevent a index being emptied if the action is replace.
Yes it would be nice to move failureThreshold, but it's a big breaking change

Copy link
Member

Choose a reason for hiding this comment

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

I was mostly wondering if the name made sense but I don't have any other in mind anyway

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we had other naming ideas with @Fluf22 but this one made the most sense, maybe advancedOptions ?

Copy link
Member

Choose a reason for hiding this comment

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

to me those seems to mostly be error settings but maybe we can generalize it indeed, policies just feel like guidelines or things like that rather than personalization

specs/ingestion/common/taskParameters.yml Outdated
Comment on lines 73 to 74
withNotifications:
name: withNotifications
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
withNotifications:
name: withNotifications
withEmailNotifications:
name: withEmailNotifications

better be descriptive than introduce bc when we add more notifications

@millotp millotp requested a review from shortcuts January 6, 2025 12:00
shortcuts
shortcuts approved these changes Jan 6, 2025
View reviewed changes
specs/ingestion/common/schemas/task.yml
required:
- email

Policies:
Copy link
Member

Choose a reason for hiding this comment

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

I was mostly wondering if the name made sense but I don't have any other in mind anyway

@millotp millotp enabled auto-merge (squash) January 6, 2025 12:33
@millotp millotp merged commit 5328ce8 into main Jan 6, 2025
28 checks passed
@millotp millotp deleted the feat/task-email branch January 6, 2025 12:43
algolia-bot added a commit that referenced this pull request Jan 6, 2025
@algolia-bot @millotp
feat(specs): add notification settings to tasks [skip-bc] (#4297) (ge…
6ea90a6 
…nerated) [skip ci]

Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
algolia-bot added a commit to algolia/algoliasearch-client-csharp that referenced this pull request Jan 6, 2025
@algolia-bot @millotp
feat(specs): add notification settings to tasks [skip-bc] (generated)
ae1f32e 
algolia/api-clients-automation#4297

Co-authored-by: algolia-bot <accounts+algolia-api-client-bot@algolia.com>
Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
algolia-bot added a commit to algolia/algoliasearch-client-go that referenced this pull request Jan 6, 2025
@algolia-bot @millotp
feat(specs): add notification settings to tasks [skip-bc] (generated)
c709537 
algolia/api-clients-automation#4297

Co-authored-by: algolia-bot <accounts+algolia-api-client-bot@algolia.com>
Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
algolia-bot added a commit to algolia/algoliasearch-client-java that referenced this pull request Jan 6, 2025
@algolia-bot @millotp
feat(specs): add notification settings to tasks [skip-bc] (generated)
9dabb5b 
algolia/api-clients-automation#4297

Co-authored-by: algolia-bot <accounts+algolia-api-client-bot@algolia.com>
Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
algolia-bot added a commit to algolia/algoliasearch-client-javascript that referenced this pull request Jan 6, 2025
@algolia-bot @millotp
feat(specs): add notification settings to tasks [skip-bc] (generated)
aaf5e87 
algolia/api-clients-automation#4297

Co-authored-by: algolia-bot <accounts+algolia-api-client-bot@algolia.com>
Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
algolia-bot added a commit to algolia/algoliasearch-client-kotlin that referenced this pull request Jan 6, 2025
@algolia-bot @millotp
feat(specs): add notification settings to tasks [skip-bc] (generated)
94a2179 
algolia/api-clients-automation#4297

Co-authored-by: algolia-bot <accounts+algolia-api-client-bot@algolia.com>
Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
algolia-bot added a commit to algolia/algoliasearch-client-php that referenced this pull request Jan 6, 2025
@algolia-bot @millotp
feat(specs): add notification settings to tasks [skip-bc] (generated)
3973ab6 
algolia/api-clients-automation#4297

Co-authored-by: algolia-bot <accounts+algolia-api-client-bot@algolia.com>
Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
algolia-bot added a commit to algolia/algoliasearch-client-python that referenced this pull request Jan 6, 2025
@algolia-bot @millotp
feat(specs): add notification settings to tasks [skip-bc] (generated)
e561c4e 
algolia/api-clients-automation#4297

Co-authored-by: algolia-bot <accounts+algolia-api-client-bot@algolia.com>
Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
algolia-bot added a commit to algolia/algoliasearch-client-ruby that referenced this pull request Jan 6, 2025
@algolia-bot @millotp
feat(specs): add notification settings to tasks [skip-bc] (generated)
5220e7b 
algolia/api-clients-automation#4297

Co-authored-by: algolia-bot <accounts+algolia-api-client-bot@algolia.com>
Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
algolia-bot added a commit to algolia/algoliasearch-client-scala that referenced this pull request Jan 6, 2025
@algolia-bot @millotp
feat(specs): add notification settings to tasks [skip-bc] (generated)
68b98ea 
algolia/api-clients-automation#4297

Co-authored-by: algolia-bot <accounts+algolia-api-client-bot@algolia.com>
Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
algolia-bot added a commit to algolia/algoliasearch-client-swift that referenced this pull request Jan 6, 2025
@algolia-bot @millotp
feat(specs): add notification settings to tasks [skip-bc] (generated)
cda4ac5 
algolia/api-clients-automation#4297

Co-authored-by: algolia-bot <accounts+algolia-api-client-bot@algolia.com>
Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shortcuts shortcuts shortcuts approved these changes

@morganleroi morganleroi Awaiting requested review from morganleroi morganleroi is a code owner automatically assigned from algolia/api-clients-automation

@Fluf22 Fluf22 Awaiting requested review from Fluf22

Assignees

@millotp millotp

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@millotp @algolia-bot @Fluf22 @shortcuts