ref(admin): add migration groups list endpoint #3231
Conversation
Codecov ReportBase: 92.30% // Head: 92.30% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #3231 +/- ##
=======================================
Coverage 92.30% 92.30%
=======================================
Files 697 697
Lines 31630 31660 +30
=======================================
+ Hits 29195 29224 +29
- Misses 2435 2436 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
I think you should also restrict this endpoint using the setting that Meredith added in #3227.
To that end, I think actually the best practice here would be to add a middleware that checks the group being referenced is allowed, and add it to every endpoint you create.
tests/admin/test_api.py
Outdated
| def sort_by_migration_id(migration: Any) -> Any: | ||
| return migration["migration_id"] |
There was a problem hiding this comment.
How come sorting by migration ID? Although by convention we use incrementing numbers for migration IDs, this is just a convention and the ordering is defined explicitly by the migration group.
There was a problem hiding this comment.
This was just to compare the lists in the assert.
| @@ -73,6 +75,27 @@ def health() -> Response: | |||
| return Response("OK", 200) | |||
|
|
|||
|
|
|||
| @application.route("/migrations/<group>/list") | |||
There was a problem hiding this comment.
Just a thought but would it be easier to return the list of all migrations by group? (basically equivalent to what the snuba migrations list command outputs)? There's really hardly any data, and the UI could easily filter them. The groups and ordering of those groups would also be present there so you wouldn't need to fire so many API calls making the frontend code a lot less complex to write.
There was a problem hiding this comment.
that would be easier, but the plan is to have some access control so users can only list the allowed groups.
There was a problem hiding this comment.
LGTM but I did add a separate testing file for clickhouse_migrations just so the test_api doesn't get to long if you wanted to add to https://github.com/getsentry/snuba/pull/3227/files
snuba/admin/views.py
Outdated
| @@ -73,6 +75,27 @@ def health() -> Response: | |||
| return Response("OK", 200) | |||
|
|
|||
|
|
|||
| @application.route("/migrations/<group>/list") | |||
| def migrations_groups(group: str) -> Response: | |||
There was a problem hiding this comment.
Is this supposed to be run on a per host basis? Like the system queries one where you select the host on which you want to run the command?
There was a problem hiding this comment.
This should connect to the migrations cluster for which there is only one node as far as I know
dbanda
force-pushed
the
dbanda/SNS-1743-list-migrations
branch
from
577a257
to
319ba83
Compare
More context in #3227
Add an endpoint
migrations/{group}/listto show the migration for a group. Returns error 400 : invalid group if thegroupis not founde.g