Skip to content

Add ManagementURL for Sourcegraph plugin #228

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

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

SimonBarendse
Copy link
Member

In the dashboard the management URL is dynamic; It contains the username: https://sourcegraph.com/users//settings/token

That's probably why it wasn't included in the first iteration of this plugin.

I just found that https://sourcegraph.com/user/settings/tokens redirects to the management URL for the currently logged in user. Sourcegraph also uses this in the 'src login' command, so it should be safe to use that here as well.

In the dashboard the management URL is dynamic; It contains the username:
https://sourcegraph.com/users/<username>/settings/token

That's probably why it wasn't included in the first iteration of
this plugin.

I just found that https://sourcegraph.com/user/settings/tokens redirects
to the management URL for the currently logged in user. Sourcegraph also
uses this in the 'src login' command, so it should be safe to use that
here as well.
Copy link
Contributor

@AndyTitu AndyTitu left a comment

Choose a reason for hiding this comment

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

I just found that https://sourcegraph.com/user/settings/tokens redirects to the management URL for the currently logged in user.

Nice touch

@SimonBarendse SimonBarendse merged commit 649cdfb into main Mar 28, 2023
@SimonBarendse SimonBarendse deleted the simon/sourcegraph-management-url branch March 28, 2023 15:58
@arunsathiya
Copy link
Contributor

This is a great addition for now, but if I may note something for our future reference, this assumes that access tokens created are for the Dotcom instance (that is, Sourcegraph.com). But for self-hosted users of the Sourcegraph software, the actual link would be https://example.com/users/settings/token. It's for that reason, I hadn't included ManagementURL in the original version:

Also, I remember that visiting https://sourcegraph.com/users/settings/token itself had resulted in a 404 at the time of my PR. It's not clear when that changed based on these recent PRs 🤷🏽‍♂️ 🤔

In any case, I am on board with us retaining the current version of ManagementURL. 💯 When in the future Shell Plugins can dynamically capture the host address/domain at the time of setup (that is, op plugin init src), the ManagementURL would need to be updated to support/adapt that new domain. In fact, even Sourcegraph's CLI out of the box assumes that everyone uses the Dotcom version:

image

@arunsathiya
Copy link
Contributor

arunsathiya commented Mar 29, 2023

Actually, I just stumbled upon this issue which demoes how Sourcegraph CLI captures the Sourcegraph instance address at runtime/setup and then generates the appropriate ManagementURL:

image

That's the approach I'd love to see Shell Plugins take one day. I am happy to take a stab at how we can tackle this as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants