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(api-idorslug): Rename Path paramaters to project_id_or_slug #69716

Merged
merged 12 commits into from
Apr 29, 2024

Conversation

iamrajjoshi
Copy link
Member

I am working on making our APIs work with slug and id in our path parameters. Over the path month, we have updated the endpoints so they can support both parameters. As a final step, we are renaming the parameters from {resource}.slug to {resource}.id_or_slug.

There will be a similar PR for organization_slug

@iamrajjoshi iamrajjoshi requested review from a team April 25, 2024 21:41
@iamrajjoshi iamrajjoshi self-assigned this Apr 25, 2024
@github-actions github-actions bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Apr 25, 2024
Copy link
Contributor

🚨 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 #discuss-dev-infra channel.

@iamrajjoshi
Copy link
Member Author

i think its complaining about frontend changes b/c i updated api-docs

@iamrajjoshi iamrajjoshi requested review from a team and removed request for a team April 26, 2024 01:16
@iamrajjoshi iamrajjoshi marked this pull request as ready for review April 26, 2024 16:42
@iamrajjoshi iamrajjoshi requested a review from a team April 26, 2024 16:42
@iamrajjoshi iamrajjoshi requested review from a team as code owners April 26, 2024 16:42
@iamrajjoshi iamrajjoshi requested a review from a team April 26, 2024 16:42
@iamrajjoshi iamrajjoshi requested review from a team as code owners April 26, 2024 16:42
@iamrajjoshi iamrajjoshi removed the request for review from a team April 26, 2024 16:42
@iamrajjoshi iamrajjoshi removed the request for review from s1gr1d April 26, 2024 16:43
Copy link
Contributor

@nhsiehgit nhsiehgit left a comment

Choose a reason for hiding this comment

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

hmmm....

Rather than a straight replace, can we roll this out in a chunks?
it's a little scary to go through all the api's in one fell swoop like this...

src/sentry/api/bases/project.py Show resolved Hide resolved
src/sentry/api/bases/project.py Show resolved Hide resolved
src/sentry/api/bases/project.py Show resolved Hide resolved
Copy link
Member

@sentaur-athena sentaur-athena left a comment

Choose a reason for hiding this comment

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

Left a few comments. Looks good in general.

@@ -198,9 +198,9 @@
}
},
{
"name": "project_slug",
"name": "projproject_id_or_slugct_slug",
Copy link
Member

Choose a reason for hiding this comment

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

typo

@@ -155,7 +155,7 @@ Retrieve a collection of feedback items.
}
```

## Feedback [/projects/<organization_slug>/<project_slug>/feedback/<feedback_id>/]
## Feedback [/projects/<organization_slug>/<project_id_or_slug>/feedback/<feedback_id>/]
Copy link
Member

Choose a reason for hiding this comment

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

oh wow! you did good.

src/sentry/api/bases/project.py Show resolved Hide resolved
src/sentry/api/bases/project.py Show resolved Hide resolved
@sentaur-athena
Copy link
Member

@nhsiehgit just to clarify Raj has rolled out the feature flag already little by little and they've been soaking in GA for a while. His current change is just renaming the variable the way it shows up in the docs. Do you think it makes sense to roll that out eventually? 🤔 I don't think it makes sense and it makes his work harder because he has to maintain two variable for docs purposes.

@nhsiehgit
Copy link
Contributor

hmm
@sentaur-athena if you're comfortable with it i can leave it to you to approve 😅
I don't think i have the context here to fully follow

@anonrig
Copy link
Contributor

anonrig commented Apr 26, 2024

I might be missing some key information here. But for the sake of people who don't know the reasoning:

  • why do we start to support both id and slug at the same time?
  • What is the performance implication of this change?
  • Does this mean we have to index both slug and id separately on the database with other fields' combinations?
  • What is the cost of operating these indexes?

@mitsuhiko
Copy link
Member

@anonrig you will find most of those questions answered here: https://www.notion.so/sentry/Make-APIs-work-with-Slug-ID-cbaf68b883b843d499191e9cc4c116d3

The page is not fully capturing this and for the sake of public readers: slug renames need to be communicated through the Hybrid Cloud RPC system and are a pain. We also persist slugs in the org level auth tokens. As a result an org slug rename currently breaks uploads and it requires synching through the hybrid cloud bridge.

We have decided a while back that it would be beneficial to support IDs in all cases so that we have a stable identifier in most communications that is unaffected by slug renames.

@sentaur-athena
Copy link
Member

Our common objects are heavily queried by id already and indexed on id. The convert_arg changes that are happening now are basically getting the ID from API and turn it to slug so most backend except this one part would not change at all.

Copy link

codecov bot commented Apr 29, 2024

Bundle Report

Changes will increase total bundle size by 1.4kB ⬆️

Bundle name Size Change
app-webpack-bundle-array-push 26.4MB 1.4kB ⬆️

@iamrajjoshi iamrajjoshi merged commit 8ec16e1 into master Apr 29, 2024
46 of 47 checks passed
@iamrajjoshi iamrajjoshi deleted the raj/api-idorslug/rename-path-params/project_slug branch April 29, 2024 21:21
@github-actions github-actions bot locked and limited conversation to collaborators May 15, 2024
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 Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants