-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
ci(api): Create a list of valid api urls #100515
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
ci(api): Create a list of valid api urls #100515
Conversation
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
| type KnownApiUrls = [ | ||
| '/', | ||
| '/', | ||
| '/accept-invite/$memberId/$token/', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The urls in this list have $ as the variable prefix, which aligns with getApiUrl()
But our routes.tsx file uses :. I think it'd be better switch the whole system over to that prefix, so routes and the static typing align.
|
would it make sense to update the makefile or package.json scripts to make it easier to run on demand |
TkDodo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great work. I’ll close my PR in favour of this one
| @@ -0,0 +1,794 @@ | |||
| /** | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this file prettier/eslint ignored or do we need annotations at the top?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't trigger any rule violations.
This file is skipped from triggering CI steps, but not omitted from the commands when they do run. I was planning to leave it that way, fewer exceptions seems easier to deal with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, just wanted to avoid opening it in an IDE and auto-save would maybe reformat it for some reason
| '/wizard/$wizardHash/', | ||
| ]; | ||
|
|
||
| export type MaybeApiPath = KnownApiUrls[number] | (string & {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should export KnownApiUrls from here only and do the rest in apiDefinition:
import type {KnownApiUrls} from './urlpatterns.generated';
export type ApiPath = KnownApiUrls;
we should also be able to remove the | (string & {}) to get real type-checking instead of type hints. I only did this so that we can work with it even though we don’t have all urls yet. Can be done in a follow-up though.
|
Just a thought: I was wondering if maybe we should have a check that catches if someone was manually altering that generated file? Not sure if that is really a concern, but we could either flag it in CI when the generated file changes, but |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #100515 +/- ##
===========================================
+ Coverage 81.15% 81.21% +0.06%
===========================================
Files 8624 8642 +18
Lines 382465 383932 +1467
Branches 24036 24036
===========================================
+ Hits 310394 311820 +1426
- Misses 71739 71780 +41
Partials 332 332 |
| @@ -1,35 +1,36 @@ | |||
| # This is used by the action https://github.com/dorny/paths-filter | |||
|
|
|||
| sentry_specific_workflow: &sentry_specific_workflow | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed, it's only used within the file
| sentry_specific_test_files: &sentry_specific_test_files | ||
| - added|modified: 'tests/js/**/*' | ||
| - added|deleted|modified: 'fixtures/search-syntax/**/*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inlined into the one callsite.
| const options = apiOptions.as<unknown>()( | ||
| '/projects/$orgSlug/$projectSlug/releases/$releaseVersion/', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had to update all the tests so they're using real routes
i tried to pick stuff that's near the top of the list, but had the properties the test cares about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the /projects one also exists, just the variables are named differently:
'/projects/$organizationIdOrSlug/$projectIdOrSlug/releases/$version/'
scttcper
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
By including the negation `'!**/*.generated.ts'` we actually were including everything else, so we were including all *.py files which is not intended. The problematic regexp was added in #100515
|
revert failed (conflict? already reverted?) -- check the logs |
This reverts commit 2d54fa1.
This script is wired into pre-commit, so that whenever a
urls.pyfile changes insentrywe re-build the list in typescript.For
getsentry, because it's a separate repo, we can't have the same automation to keep the files in sync :(. So that list is broken out into it's own file and will need to manually be managed for now.Pre commit
I triggered the pre-commit step on any urls.py file becaues we have our main
src/sentry/api/urls.pyfile, but also a few places that contribute imports likesrc/sentry/workflow_engine/endpoints/urls.pyandsrc/sentry/notifications/platform/api/endpoints/urls.py. I'd rather not have these extra urls.py file at all, and have all api routes be defined in one py file. But even if i go fix it now, someone is bound to add an import to import their route definitions again in the future.Ok, so we have a pre-commit. It will generate/update the ts file.
CI tasks
Changing the TS file does not trigger a full eslint/jest run in the frontend CI. Also it does not add the "Scope: Frontend" label to the PR.
Only the TypeScript command is executed when the generated file changes. This will catch cases where someone trys to remove an api that is still in use (assuming all api call sites are statically typed, which they are not yet). Adding a new api endpoint in python will trigger TS, but shouldn't ever fail.
If someone manages to add a new endpoint and the ts file doesn't re-generate then it won't be the end of the world. In the short term we'll just re-gen the file.
Testing the Script
There's about ~800 routes that the script identified. I got an llm to generate all the regexp that splits url patterns into N strings. It appears to be handling cases where a url segment is named, like
/(?P<issue_id>[^/]+)/or has alternates like/(issue|group)/.With these examples working i didn't look through all 800 urls to validate them, I also didn't look for anything that's missing. As we statically type all our frontend code using this list, if something is wrong we can revisit the script and fix it up.
There are however tests (llm generated) that are passing.
Running the script
The generated file has the command embedded inside it, and we can also see from the pre-commit-config:
python3 -m tools.api_urls_to_typescriptwill do it.It can also be run with a flag:
python3 -m tools.api_urls_to_typescript list-> output to the terminal a list of urls, no formattingRelated to #97155
Note
Add Python tool to generate TS typings for API URLs from Django URL patterns, commit the generated file, add tests, and wire into pre-commit and CI filters.
tools/api_urls_to_typescript.pyto derive routes from Djangourlpatternsand emitstatic/app/api/urlpatterns.generated.ts(withKnownApiUrlsandMaybeApiPath).python3 -m tools.api_urls_to_typescript list.static/app/api/urlpatterns.generated.tscontaining the enumerated API paths.tests/tools/test_api_urls_to_typescript.pycoveringregexp_to_routes(named groups, alternates, optional parts, complex patterns).gen-ts-api-urlshook to regenerate TS routes onurls.pychanges..github/file-filters.ymlto broaden TS/JS globs and exclude*.generated.ts; add frontend workflow anchor and include in triggers.frontend.ymljob outputs to exposetypecheckable_rules_changed.Written by Cursor Bugbot for commit 82c5d6a. This will update automatically on new commits. Configure here.