Skip to content

feat: enhance provider and deduplication rule provisioning logic #4399

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

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

tuantran0910
Copy link
Contributor

@tuantran0910 tuantran0910 commented Apr 3, 2025

Closes #4401

📑 Description

When Keep starts or restarts, it follows these steps to manage provider configurations:

  1. Read Configurations: Loads provider definitions from either the KEEP_PROVIDERS environment variable or YAML files in the KEEP_PROVIDERS_DIRECTORY.
  2. Create New Providers: Installs any providers listed in the configuration that are not already present.
  3. Update When Changed: If an existing provider's configuration has changed, Keep reapplies the configuration, including deduplication rules. If errors occur during this update, changes are automatically rolled back.
  4. Delete Providers: Deletes any currently installed providers that are not found in the loaded configuration.

✅ Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

Copy link

vercel bot commented Apr 3, 2025

@tuantran0910 is attempting to deploy a commit to the KeepHQ Team on Vercel.

A member of the Team first needs to authorize it.

@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Apr 3, 2025
@tuantran0910 tuantran0910 marked this pull request as draft April 3, 2025 19:06
@dosubot dosubot bot added Feature A new feature Provider Providers related issues labels Apr 3, 2025
@tuantran0910 tuantran0910 marked this pull request as ready for review April 4, 2025 06:50
@tuantran0910
Copy link
Contributor Author

tuantran0910 commented Apr 5, 2025

Hmm, storing the hash value when provisioning in local file storage might not be a good way when we have many backend replicas as mentioned in this issue #4398. Solved by use other SecretManager type like k8s :D

@shahargl
Copy link
Member

shahargl commented Apr 7, 2025

hey @tuantran0910 - although we want to get this functionality, we feel it's a bit over-engineering here. let's sync if see if there is simpler solution for that.

@tuantran0910
Copy link
Contributor Author

hey @tuantran0910 - although we want to get this functionality, we feel it's a bit over-engineering here. let's sync if see if there is simpler solution for that.

Hi @shahargl, I completely agree with you. We always aim for the simplest solution to our problems. I don’t have much experience before, so I would really appreciate any advice you could share. Thanks! :D

@tuantran0910 tuantran0910 marked this pull request as draft April 12, 2025 13:11
@tuantran0910
Copy link
Contributor Author

Hi @talboren, @shahargl, I've simplified the provider management logic (as described in the issue/PR description). The process is now:

  1. Iterate through the providers defined in the configuration.
  2. If a provider doesn't exist locally, install it.
  3. If a provider already exists, update it based on the configuration.
  4. After processing all configured providers, delete any existing providers that were not found in the current configuration (stale providers).

How do you guys think about this strategy ?

@tuantran0910 tuantran0910 marked this pull request as ready for review April 12, 2025 18:06
@dosubot dosubot bot added Documentation Improvements or additions to documentation hacktoberfest-accepted labels Apr 12, 2025
Copy link

codecov bot commented Apr 12, 2025

Codecov Report

Attention: Patch coverage is 31.25000% with 11 lines in your changes missing coverage. Please review.

Project coverage is 46.76%. Comparing base (d39afd3) to head (dfb7b70).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...t_deduplicator/deduplication_rules_provisioning.py 27.27% 8 Missing ⚠️
keep/api/core/db.py 40.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4399      +/-   ##
==========================================
- Coverage   46.77%   46.76%   -0.01%     
==========================================
  Files         164      164              
  Lines       16841    16838       -3     
==========================================
- Hits         7877     7874       -3     
  Misses       8964     8964              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tuantran0910 tuantran0910 changed the title feat: enhance deduplication rules provisioning with provider support and environment configuration feat: enhance provider and deduplication rule provisioning logic Apr 12, 2025
@talboren talboren requested review from shahargl and talboren April 14, 2025 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Improvements or additions to documentation Feature A new feature hacktoberfest-accepted Provider Providers related issues size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: Provisioned provider not updated with new configurations
3 participants