fix(hc): Adds support for region URL overrides in CLI #2003
Conversation
src/config.rs
Outdated
| _ => bail!( | ||
| "Two different url values supplied: `{token_url}` (from token), `{default_url}`." | ||
| "URL must match one of the provided URLs on the authentication token: `{token_url}` or `{region_url}`, received: `{default_url}`." |
There was a problem hiding this comment.
This could probably use some better wording, but it feels appropriate to include the region URL here as an option for the user to select it.
There was a problem hiding this comment.
Also this is a little quirky. If the region_url and token_url match, you'll essentially get a message with the same expected URL twice. I can make this exhaustive at the expense of adding multiple distinct error message cases.
There was a problem hiding this comment.
Yeah, I think we should add the distinct error message cases; might be confusing if we list the same URL twice in the same error message
| $ sentry-cli --auth-token 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 --url http://localhost:8000 sourcemaps upload --org nomatch test_path | ||
| ? failed | ||
| error: the following required arguments were not provided: | ||
| <PATHS>... | ||
| error: Two different org values supplied: `sentry` (from token), `nomatch`. | ||
|
|
||
| Usage: sentry-cli[EXE] sourcemaps upload <PATHS>... | ||
|
|
||
| For more information, try '--help'. | ||
| Add --log-level=[info|debug] or export SENTRY_LOG_LEVEL=[info|debug] to see more output. |
There was a problem hiding this comment.
This test wasn't actually asserting the URL matches correctly. It appears that the upload <paths> check occurs before the URL checking logic is run.
There was a problem hiding this comment.
I think we might want to go a different direction with this PR.
Specifically, I would propose that we simply change the logic so that when an org auth token is used, we route all requests to the region URL specified in the token (not just the region specific ones). Simultaneously, we should deprecate the --url option when the org auth token is used, since it is redundant to specify both a --url and an auth token.
Instead, if a user provides a --url argument with an org auth token, we should print a warning to let them know that using --url with an org auth token is deprecated, and we should simply ignore the --url argument.
I can prepare some PRs to implement this functionality
| $ sentry-cli --auth-token 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 --url http://localhost:8000 sourcemaps upload --org nomatch test_path | ||
| ? failed | ||
| error: the following required arguments were not provided: | ||
| <PATHS>... | ||
| error: Two different org values supplied: `sentry` (from token), `nomatch`. | ||
|
|
||
| Usage: sentry-cli[EXE] sourcemaps upload <PATHS>... | ||
|
|
||
| For more information, try '--help'. | ||
| Add --log-level=[info|debug] or export SENTRY_LOG_LEVEL=[info|debug] to see more output. |
|
Actually, we currently cannot send all requests to the region endpoint, as this breaks the |
There was a problem hiding this comment.
I would say this generally looks good, if you would like to go with this option.
I would however slightly prefer something like #2005 since I think it would make the CLI more maintainable if we reduce the number of user-configurable options, and if I recall correctly from our meeting, we anyways wanted to move towards sending all requests directly to the region
src/config.rs
Outdated
| _ => bail!( | ||
| "Two different url values supplied: `{token_url}` (from token), `{default_url}`." | ||
| "URL must match one of the provided URLs on the authentication token: `{token_url}` or `{region_url}`, received: `{default_url}`." |
There was a problem hiding this comment.
Yeah, I think we should add the distinct error message cases; might be confusing if we list the same URL twice in the same error message
@szokeasaurusrex Agreed. I do think we surface the base control silo URL in our org auth tokens as well, so maybe we can add some logic that defaults to the region whenever possible for all calls except for the very few control-silo specific calls that we can explicitly mark as control-silo bound requests. I can help audit all of the calls to ensure those get marked/routed accordingly. |
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Adds support for region-specific URL overrides, so long as the region URL matches the
region_urlencoded on a supplied organization authentication token.