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

feat: Logic of different modes for webhook #9865

Merged
merged 6 commits into from
Aug 28, 2024
Merged

feat: Logic of different modes for webhook #9865

merged 6 commits into from
Aug 28, 2024

Conversation

gt2345
Copy link
Contributor

@gt2345 gt2345 commented Aug 26, 2024

Ticket

MD-479

Description

Part of workload alerting project.
ERD link: https://hpe-aiatscale.atlassian.net/wiki/spaces/ENGINEERIN/pages/1666809868/Workload+Alerting+ERD

Expand the experiment config -> integrations -> webhooks

integrations:
  webhooks:
    webhook_name:
      - test-webhook

Below is a table for which group of experiments can trigger which mode of webhooks.

workspace \ mode Specific Workspace
workspace defined match workspace && match config all experiments within workspace
workspace null match config all experiments

Test Plan

Unit tests and e2e tests should be able to cover it.
To test from WebUI:

  • Create a webhook with workspace ID
  • Triggering the webhook with experiments within that workspace should work
  • The webhook should not be triggered with experiments outside of that workspace
  • Create a webhook with mode specific
  • experiment should only trigger that webhook with matching config

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

@gt2345 gt2345 requested review from a team as code owners August 26, 2024 15:09
@cla-bot cla-bot bot added the cla-signed label Aug 26, 2024
Copy link

netlify bot commented Aug 26, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit f5831ef
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66cf35d660cda90008e796ed

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

Attention: Patch coverage is 60.33058% with 48 lines in your changes missing coverage. Please review.

Project coverage is 54.65%. Comparing base (fb95df8) to head (f5831ef).
Report is 11 commits behind head on main.

Files Patch % Lines
master/pkg/schemas/zgen_schemas.go 0.00% 15 Missing ⚠️
master/internal/experiment/authz_rbac.go 0.00% 12 Missing ⚠️
master/internal/api_tasks.go 67.74% 10 Missing ⚠️
master/internal/webhooks/postgres_webhook.go 85.45% 8 Missing ⚠️
master/internal/core_task.go 0.00% 1 Missing ⚠️
master/internal/db/postgres_test_utils.go 50.00% 1 Missing ⚠️
master/internal/webhooks/singleton.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9865      +/-   ##
==========================================
- Coverage   54.65%   54.65%   -0.01%     
==========================================
  Files        1261     1261              
  Lines      156226   156305      +79     
  Branches     3592     3591       -1     
==========================================
+ Hits        85382    85424      +42     
- Misses      70712    70749      +37     
  Partials      132      132              
Flag Coverage Δ
backend 45.17% <60.33%> (+0.01%) ⬆️
harness 72.60% <ø> (ø)
web 54.35% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
master/internal/api_experiment.go 56.80% <100.00%> (ø)
master/internal/api_task.go 70.37% <100.00%> (ø)
master/internal/api_trials.go 55.48% <100.00%> (ø)
master/internal/core_task.go 0.00% <0.00%> (ø)
master/internal/db/postgres_test_utils.go 76.92% <50.00%> (-0.19%) ⬇️
master/internal/webhooks/singleton.go 38.46% <0.00%> (ø)
master/internal/webhooks/postgres_webhook.go 60.56% <85.45%> (+3.23%) ⬆️
master/internal/api_tasks.go 49.58% <67.74%> (+0.53%) ⬆️
master/internal/experiment/authz_rbac.go 0.50% <0.00%> (-0.02%) ⬇️
master/pkg/schemas/zgen_schemas.go 1.11% <0.00%> (-0.02%) ⬇️

... and 4 files with indirect coverage changes

# Only need a spot check we get the default / slack responses.
# Further tested in integrations.
assert "TASK_LOG" in responses[default_path]
assert "This log matched the regex" in responses[slack_path]
assert "TASK_LOG" in responses[default_path]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a duplicate line?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe specific_path ?

Copy link
Contributor

@MikhailKardash MikhailKardash left a comment

Choose a reason for hiding this comment

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

Python code LGTM

Copy link
Contributor

@salonig23 salonig23 left a comment

Choose a reason for hiding this comment

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

LGTM! Love the detailed test cases, good job!

@gt2345 gt2345 merged commit 54b6165 into main Aug 28, 2024
80 of 95 checks passed
@gt2345 gt2345 deleted the gt/479-webhook-mode branch August 28, 2024 15:07
tara-hpe added a commit that referenced this pull request Sep 16, 2024
Support workload alerting, different webhook modes, e.g., #9865
@tara-hpe tara-hpe mentioned this pull request Sep 16, 2024
5 tasks
tara-hpe added a commit that referenced this pull request Sep 17, 2024
Support workload alerting, different webhook modes, e.g., #9865
tara-hpe added a commit that referenced this pull request Sep 18, 2024
Support workload alerting, different webhook modes, e.g., #9865
tara-hpe added a commit that referenced this pull request Sep 19, 2024
Support workload alerting, different webhook modes, e.g., #9865
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants