-
-
Notifications
You must be signed in to change notification settings - Fork 230
docs(id-support): Document that -p
and -o
arguments accept slugs and IDs
#2098
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
Conversation
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 am lacking context on the API changes to enable users to use organization/project IDs rather than slugs that you made @iamrajjoshi (in the future, please link changes you are referring to in the PR so that people unfamiliar with your direct work have context on changes you propose), so please be aware that my review is based on a limited understanding of these changes.
Are you planning to support organization/project IDs for all API endpoints in Sentry, or only some of them? And what endpoints are supported now?
From the changes you are proposing, it would seem that only the endpoint hit by the event list
command is supported; however, in my testing, I noticed that the issues list
, files upload
, and releases upload
commands can all also take org/project IDs, even though this change does not address the issues list
command. Given this PR in the Sentry repo, which claims to update all endpoints to support org/project IDs, it would seem reasonable to infer that all Sentry APIs (and hence, all Sentry CLI commands) should now support org/project IDs; however, this is not the case, since calling sourcemaps upload
with a project ID instead of a project slug results in the following error:
error: API request failed
caused by: sentry reported an error: One or more projects are invalid (http status: 400)
All of this to say, I am very confused about why you are proposing to add so much complexity to the Sentry CLI code in the form of two new functions that are almost identical to existing functions, only so that one of the many commands that support IDs for org/projects have this stated in their help text.
The simplest solution here would be to wait until Sentry supports org/project IDs for all APIs1 (or at least, all APIs that the CLI accesses), and then simply update the existing org_arg
and project_arg
functions to indicate that all commands that take these arguments support a slug or ID.
So, let's please wait until all Sentry endpoints actually support org/project endpoints, then just update the existing functions with the new help text. Merging this PR as is would only add unnecessary complexity to the CLI code, and since the CLI code is already way to complex as is, I strongly oppose making it even more complex unless there is a very good reason to do so.
There is no downside to delaying the merging of this PR, since it is, in effect, only a documentation change to the help text, and it does not add any real functionality. The CLI already supports project and org IDs for the event list
command.
Footnotes
-
If, for some reason, we do not plan to support IDs for all Sentry APIs, I would suggest we make no changes in the CLI since we already support IDs everywhere the Sentry API does, and this can simply remain a hidden feature. It would probably confuse users though if only some commands supported IDs, so I would advocate allowing IDs for all Sentry API endpoints so they can be universally supported in the CLI. ↩
"dateCreated": "2024-07-11T16:45:57Z", | ||
"crashFile": null | ||
} | ||
] |
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.
] | |
] | |
Let's make sure this file ends in a newline
event list
Command-p
and -o
arguments accept slugs and IDs
.value_name("ORG") | ||
.long("org") | ||
.short('o') | ||
.value_parser(validate_org) |
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.
You are still using the same value parser as the org
argument here. I think the validation itself should be correct, but if you pass an invalid value, it prints "Invalid value for organization. Use the URL slug and not the name!"
, which is misleading.
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.
With the changes I requested, we would no longer have a separate org_arg_with_id
function.
Instead, we would need to update the existing validate_org
value parser to state that an organization slug or ID needs to be provided.
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.
Of course, that also (like the rest of the changes) can only be done once all Sentry APIs support org or project IDs
Hey @szokeasaurusrex , thanks for the review! You are right, I should have added a bit more context here about the API changes made for folks less familiar (a shipped post is incoming).
We now support Ids and Slugs in the path parameters of all our APIs. This has been tested via tests for all the endpoints. I just looked into the My plan isn't to add extra complexity. I wanted to roll out this documentation changes slowly, while testing all the commands to make sure they work with both identifiers. I was planning on updating the help strings in batches as I wrote tests and tested all the commands. I added these additional two functions temporarily until all the commands had been tested, then deleted them. That being said, if we want to make this change all together, let me find all APIs that the CLI hits and check if there are any other commands that hit endpoints that pass slugs as body params (which i am fairly certain shouldn't be many) and update those endpoints before make a blanket pr to update the docs. |
Are we planning to support IDs for body parameters, as well?
Please let me know if you have any questions! |
I am planning to add support for api body params that are used by the CLI as of now. Then I'll just change the help strings in bulk. After I rename, if someone is adding functionality to CLI then they will know that they should consider id as well and create the support for API if needed. |
Closing this PR for now, will make another one to change the help string sin bulk. |
With the API changes we made to support ids as well as slugs, we get the added bonus that our CLI can also support ids.
I verified this locally for a few commands and it all seems to be working. This is the first of a couple prs to update the CLI to reflect this new support.
In this pr, i add id support for the
events list
command. To do this, I defined two new args (which are identical but with a better help message) and wrote tests to test this new feature. The API has already been verified to work with both ids and slugs.rough spec