Jinja2 based routes #1319

iskhakov · 2023-02-10T13:39:16Z

What this PR does

This PR adds the new way to set up routes using jinja2 templating language

Which issue(s) this PR fixes

Checklist

Tests updated
Documentation added
CHANGELOG.md updated

grafana-plugin/src/containers/ChannelFilterForm/ChannelFilterForm.tsx

grafana-plugin/src/models/channel_filter/channel_filter.types.ts

engine/apps/api/serializers/channel_filter.py

engine/apps/alerts/models/channel_filter.py

engine/apps/alerts/models/alert_group_log_record.py

vstpme

LGTM, probably a good idea to add a couple tests on this, e.g. somewhere here:

oncall/engine/apps/alerts/tests/test_channel_filter.py

Line 9 in 19f1491

    
           def test_channel_filter_select_filter(make_organization, make_alert_receive_channel, make_channel_filter):

engine/apps/api/serializers/channel_filter.py

Konstantinov-Innokentii · 2023-03-06T14:36:21Z

engine/apps/alerts/migrations/0010_channelfilter_filtering_term_type.py

+        migrations.AddField(
+            model_name='channelfilter',
+            name='filtering_term_type',
+            field=models.IntegerField(choices=[(0, 'regex'), (1, 'jinja2')], default=None, null=True),


Can we have this field non-nullable, but with default value 0? With that we can get rid of to_representation hack

I decided not use this kind of migrations to make the release smoother for us and the oss customers (check here for more details)

I think it's valid tradeoff to experience 500's to have more consistent database and codebase. With nullable we need a lot of code to show this field in private/public API correctly and to check for null to write to that field from API.

engine/apps/api/serializers/channel_filter.py

Konstantinov-Innokentii · 2023-03-06T14:40:49Z

engine/apps/api/serializers/channel_filter.py

@@ -56,22 +57,6 @@ def get_slack_channel(self, obj):
            "id": obj.slack_channel_pk,
        }

-    def validate(self, attrs):


Where jinja template is validated, when route is created/modified?

This was used to ensure that the routes are based on the unique regular expressions. I decided to drop uniqueness, if user wants to create exactly same route which will never get it's alert, let them do that, may be they'll want to modify it later.

Konstantinov-Innokentii · 2023-03-06T14:41:14Z

engine/apps/public_api/serializers/routes.py

@@ -176,20 +176,6 @@ def create(self, validated_data):

        return instance

-    def validate(self, attrs):


Same question as in internal API - where jinja template is validated?

Konstantinov-Innokentii · 2023-03-06T14:42:46Z

engine/apps/alerts/models/channel_filter.py

+                return False
+        if self.filtering_term is not None and self.filtering_term_type == ChannelFilter.FILTERING_TERM_TYPE_REGEX:
+            try:
+                return re.search(self.filtering_term, json.dumps(value))


json.dumps(value) is introduced here, could you please describe why?

it's moved from here https://github.com/grafana/oncall/pull/1319/files#diff-5b78eae626bcf20076ff2f5f25daa3f5703ff8dd9336e5d3e3dbbd7aa827d6b2L116

Konstantinov-Innokentii · 2023-03-06T14:46:26Z

Great idea, left comments. It will be great to support new filter term type in public api and in the terraform provider. Also, i think it is needed to provide some info in docs about jinja based routes.

iskhakov · 2023-03-07T07:53:14Z

engine/apps/alerts/migrations/0010_channelfilter_filtering_term_type.py

+        ),
+        # Adding same default value on the database level
+        # for database backwards-compatibility with older versions of code
+        AddDefaultValue(


this is possible with RunSQL to insert default values, but we'll need to do that manually for each database. django_add_default_value does that for us

I didn't get that idea. Why we need AddDefaultValue, when we have default=0?

I left the better comment in the code
for more details I recommend to read this article

# What this PR does This PR adds the new way to set up routes using jinja2 templating language <img width="1174" alt="Screenshot 2023-03-06 at 22 11 13" src="https://user-images.githubusercontent.com/2262529/223134053-69d43c47-bb2a-4790-a16d-767425017a76.png"> <img width="1175" alt="Screenshot 2023-03-06 at 22 11 34" src="https://user-images.githubusercontent.com/2262529/223134070-1e5ef82f-021c-4d5d-b255-b19bb3445641.png"> ## Which issue(s) this PR fixes ## Checklist - [ ] Tests updated - [ ] Documentation added - [ ] `CHANGELOG.md` updated

iskhakov added 2 commits February 10, 2023 21:38

Add jinja2 based routing

7947941

Add migrations

1833fcd

iskhakov changed the title ~~Iskhakov/jinja2 routes~~ Jinja2 based routes Feb 10, 2023

Try using enum

2c71748

iskhakov commented Feb 10, 2023

View reviewed changes

grafana-plugin/src/containers/ChannelFilterForm/ChannelFilterForm.tsx Show resolved Hide resolved

maskin25 reviewed Feb 10, 2023

View reviewed changes

grafana-plugin/src/models/channel_filter/channel_filter.types.ts Outdated Show resolved Hide resolved

joeyorlando reviewed Feb 16, 2023

View reviewed changes

engine/apps/api/serializers/channel_filter.py Outdated Show resolved Hide resolved

joeyorlando reviewed Feb 16, 2023

View reviewed changes

engine/apps/alerts/models/channel_filter.py Show resolved Hide resolved

joeyorlando reviewed Feb 16, 2023

View reviewed changes

engine/apps/alerts/models/alert_group_log_record.py Outdated Show resolved Hide resolved

iskhakov and others added 5 commits February 17, 2023 18:04

Merge branch 'dev' into iskhakov/jinja2-routes

5730e4c

Improve design

9448fa5

Add design improvements

478f72b

Use one model field to store both jinja2 and regex terms

048a701

Fontend cleanup

0d48c01

iskhakov requested a review from maskin25 March 6, 2023 11:22

iskhakov added 2 commits March 6, 2023 19:27

Fix button name

2630778

Refactor ChannelFilter.str_for_clients function

a109b0a

iskhakov marked this pull request as ready for review March 6, 2023 12:34

iskhakov requested review from a team March 6, 2023 12:34

vstpme approved these changes Mar 6, 2023

View reviewed changes

engine/apps/api/serializers/channel_filter.py Outdated Show resolved Hide resolved

Fixes

7707f3d

Konstantinov-Innokentii reviewed Mar 6, 2023

View reviewed changes

engine/apps/api/serializers/channel_filter.py Outdated Show resolved Hide resolved

Konstantinov-Innokentii reviewed Mar 6, 2023

View reviewed changes

Add docs

2cc3b46

Add default value on the database level to prevent release crash

5ec760b

iskhakov commented Mar 7, 2023

View reviewed changes

iskhakov added 4 commits March 8, 2023 11:53

Add validation

351277c

Add better comment

401b1b1

Merge branch 'dev' into iskhakov/jinja2-routes

f77b4bc

Add tests

d430a16

Konstantinov-Innokentii approved these changes Mar 8, 2023

View reviewed changes

Add changes to api and docs

4ee0652

iskhakov enabled auto-merge March 8, 2023 08:10

iskhakov disabled auto-merge March 8, 2023 08:10

iskhakov added 2 commits March 8, 2023 16:13

Edit test_routes.py

bc2e3a7

Edit test_routes.py

17675e3

iskhakov enabled auto-merge March 8, 2023 08:31

iskhakov disabled auto-merge March 8, 2023 08:31

iskhakov enabled auto-merge March 8, 2023 08:31

iskhakov disabled auto-merge March 8, 2023 08:41

iskhakov merged commit 2e63a9f into dev Mar 8, 2023

iskhakov deleted the iskhakov/jinja2-routes branch March 8, 2023 08:42

nijave mentioned this pull request Mar 10, 2023

On-Call Jinja route support grafana/terraform-provider-grafana#849

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jinja2 based routes #1319

Jinja2 based routes #1319

iskhakov commented Feb 10, 2023 •

edited

Loading

vstpme left a comment •

edited

Loading

Konstantinov-Innokentii Mar 6, 2023 •

edited

Loading

iskhakov Mar 6, 2023

Konstantinov-Innokentii Mar 7, 2023

Konstantinov-Innokentii Mar 6, 2023 •

edited

Loading

iskhakov Mar 7, 2023

Konstantinov-Innokentii Mar 6, 2023

Konstantinov-Innokentii Mar 6, 2023

iskhakov Mar 6, 2023

Konstantinov-Innokentii commented Mar 6, 2023

iskhakov Mar 7, 2023

Konstantinov-Innokentii Mar 7, 2023

iskhakov Mar 8, 2023 •

edited

Loading

		@@ -176,20 +176,6 @@ def create(self, validated_data):

		return instance

		def validate(self, attrs):

Jinja2 based routes #1319

Jinja2 based routes #1319

Conversation

iskhakov commented Feb 10, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes

Checklist

vstpme left a comment • edited Loading

Choose a reason for hiding this comment

Konstantinov-Innokentii Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Konstantinov-Innokentii Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Konstantinov-Innokentii commented Mar 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iskhakov Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

iskhakov commented Feb 10, 2023 •

edited

Loading

vstpme left a comment •

edited

Loading

Konstantinov-Innokentii Mar 6, 2023 •

edited

Loading

Konstantinov-Innokentii Mar 6, 2023 •

edited

Loading

iskhakov Mar 8, 2023 •

edited

Loading