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(crons): Add monitor slug to APIs #45294

Merged

Conversation

evanpurkhiser
Copy link
Member

  • Returns slug as part of the serialized monitor
  • Allows slug to be specified for creation and updates

@evanpurkhiser evanpurkhiser requested review from a team as code owners March 2, 2023 02:24
@evanpurkhiser evanpurkhiser requested review from a team and rjo100 and removed request for a team March 2, 2023 02:24
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 2, 2023
@codecov
Copy link

codecov bot commented Mar 2, 2023

Codecov Report

Merging #45294 (69481a9) into master (285c9be) will increase coverage by 23.12%.
The diff coverage is 100.00%.

❗ Current head 69481a9 differs from pull request most recent head 93b2f82. Consider uploading reports for the commit 93b2f82 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #45294       +/-   ##
===========================================
+ Coverage   57.10%   80.22%   +23.12%     
===========================================
  Files        4689     4724       +35     
  Lines      198189   198819      +630     
  Branches    12006    12007        +1     
===========================================
+ Hits       113173   159504    +46331     
+ Misses      84754    39053    -45701     
  Partials      262      262               
Impacted Files Coverage Δ
src/sentry/api/endpoints/organization_monitors.py 56.71% <ø> (+22.38%) ⬆️
...ntry/api/endpoints/organization_monitor_details.py 94.91% <100.00%> (+49.30%) ⬆️
src/sentry/api/serializers/models/monitor.py 100.00% <100.00%> (+23.07%) ⬆️
src/sentry/api/validators/monitor.py 92.70% <100.00%> (+50.60%) ⬆️
src/sentry/models/monitor.py 96.61% <100.00%> (+39.83%) ⬆️
...ts/events/interfaces/frame/stacktraceLinkModal.tsx 88.88% <0.00%> (-1.81%) ⬇️
static/app/views/onboarding/index.tsx 61.44% <0.00%> (-0.75%) ⬇️
...c/sentry/api/endpoints/organization_events_meta.py 91.07% <0.00%> (-0.16%) ⬇️
src/sentry/search/base.py 76.47% <0.00%> (ø)
src/sentry/projectoptions/defaults.py 100.00% <0.00%> (ø)
... and 1573 more

@@ -89,6 +89,7 @@ def validate(self, attrs):
class MonitorValidator(serializers.Serializer):
project = ProjectField(scope="project:read")
name = serializers.CharField()
slug = serializers.RegexField(r"^[a-z0-9_\-]+$", max_length=50, required=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally we could move this to a shared constant but probably outside the scope of this PR. just like a ALPHANUMERIC_SLUG_REGEX

Copy link
Contributor

Choose a reason for hiding this comment

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

it's used in 4 other places

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a lot of inconsistent slug = serializer.RegexField

@@ -44,6 +44,16 @@ def test_name(self):
monitor = Monitor.objects.get(id=monitor.id)
assert monitor.name == "Monitor Name"

def test_slug(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

worth adding a test where slug is specified but is None or "", I don't think the regex checker will allow it in MonitorValidator. generally we allow None values as equivalent to not specifying an optional param

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated. It fails with 'cannot be blank'

@evanpurkhiser evanpurkhiser force-pushed the evanpurkhiser/feat-crons-add-monitor-slug-to-apis branch from 139170b to f13048e Compare March 2, 2023 19:32
@evanpurkhiser evanpurkhiser force-pushed the evanpurkhiser/feat-crons-add-monitor-slug-to-apis branch from f13048e to 69481a9 Compare March 2, 2023 21:36
@evanpurkhiser evanpurkhiser force-pushed the evanpurkhiser/feat-crons-add-monitor-slug-to-apis branch from 69481a9 to 535f4a2 Compare March 2, 2023 23:36
@evanpurkhiser evanpurkhiser force-pushed the evanpurkhiser/feat-crons-add-monitor-slug-to-apis branch from 535f4a2 to 3463d50 Compare March 3, 2023 00:11
 * Returns slug as part of the serialized monitor
 * Allows slug to be specified for creation and updates
@evanpurkhiser evanpurkhiser force-pushed the evanpurkhiser/feat-crons-add-monitor-slug-to-apis branch from 3463d50 to 93b2f82 Compare March 3, 2023 19:18
@evanpurkhiser evanpurkhiser merged commit ee08eb5 into master Mar 3, 2023
@evanpurkhiser evanpurkhiser deleted the evanpurkhiser/feat-crons-add-monitor-slug-to-apis branch March 3, 2023 19:54
@github-actions github-actions bot locked and limited conversation to collaborators Mar 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants