Rs/add simple slack notification #478
Conversation
github-actions
bot
added
topic: build
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #478 +/- ##
==========================================
- Coverage 84.79% 84.09% -0.70%
==========================================
Files 38 39 +1
Lines 1151 1201 +50
==========================================
+ Hits 976 1010 +34
- Misses 175 191 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I might be mistaking but it looks like you forgot to remove cv2 and/or pillow + the extra dependencies from Docker |
| @@ -10,7 +10,6 @@ ENV PYTHONPATH="/app" | |||
| # Install curl | |||
| RUN apt-get -y update \ | |||
| && apt-get -y install curl libmagic1 \ | |||
| && apt-get clean \ | |||
There was a problem hiding this comment.
Can you please revert this please?
| @@ -134,6 +139,18 @@ async def create_detection( | |||
| org = cast(Organization, await organizations.get(token_payload.organization_id, strict=True)) | |||
| if org.telegram_id: | |||
| background_tasks.add_task(telegram_client.notify, org.telegram_id, det.model_dump_json()) | |||
|
|
|||
| if slack_client.is_enabled: | |||
| org = cast(Organization, await organizations.get(token_payload.organization_id, strict=True)) | |||
There was a problem hiding this comment.
Let's avoid the double call if telegram and slack client are enabled
There was a problem hiding this comment.
hum I don't understand, what is the issue here ?
| organizations: OrganizationCRUD = Depends(get_organization_crud), | ||
| token_payload: TokenPayload = Security(get_jwt, scopes=[UserRole.ADMIN]), | ||
| ) -> Organization: | ||
| telemetry_client.capture( |
There was a problem hiding this comment.
This isn't relevant for product analytics, no need to add the event capture :)
| @@ -17,6 +17,7 @@ pydantic = ">=2.0.0,<3.0.0" | |||
| pydantic-settings = ">=2.0.0,<3.0.0" | |||
| requests = "^2.32.0" | |||
| PyJWT = "^2.8.0" | |||
| bcrypt = "<4.0.0" | |||
There was a problem hiding this comment.
Can you elaborate on this one? (it's already in the passlib extra) did you encounter a bug with the lockfile?
| @@ -55,6 +55,7 @@ services: | |||
| - S3_ACCESS_KEY=fake | |||
| - S3_SECRET_KEY=fake | |||
| - S3_REGION=us-east-1 | |||
| - SLACK_HOOK=${SLACK_HOOK} | |||
There was a problem hiding this comment.
if I understand this correctly, this is the admin org slack hook?
If so I think it shouldn't be initialized in the DB initialization script + validated in the config
If not, let's remove it?
There was a problem hiding this comment.
this is used only for testing purpose
There was a problem hiding this comment.
boto3 was already on the version you specified, why is the entire lockfile updated? can you please revert? Check my comment in pyproject but if bcrypt is indeed not needed, we don't need to change the lockfile
(please don't bump versions without a reason)
Give the possibility to activate slack notification into a channel (one channel per organisation)
=> Add an endpoint in the organizations/ path in order to activate the slack notification by registering the SLACK_HOOK
=> Create a SLACK service, similar to the Telegram service
=> modify the create_detection endpoint in on order to add the notification creation