Conversation
github-actions
bot
added
team/product-platform
team/source
To bring Gerrit support more in line with other code hosts and because a customer recently ran into this limitation, this PR adds support for the SSH flag. The diff is mostly straightforward, with two things to watch out for: - The clone URL construction in makeRepo was wrong previously, it missed the `/a/`. This is only used for visuals, but still annoying. So I moved the whole construction into here from the gitserver cloneURL package. - The SSH hostname and port are configurable in Gerrit, so to construct the right URL we need to fetch some instance metadata. This would be costly to do in the gitserver method, so we persist all the info needed to construct clone URLs "offline" during the cloning process by storing all the data for HTTP and SSH clones on the repo metadata column. This is mostly in line with other code hosts as well, see GitLab right above in the gitserver/cloneurl package. Closes https://github.com/sourcegraph/sourcegraph/issues/60597 ## Test plan Added tests for the various stages, recreated recorded responses, and tried both HTTP and SSH cloning locally, both worked with out Gerrit instance.
| - Auth providers now support a `noSignIn` option that, when set to true, will hide the auth provider from the sign in page, but still allow users to connect the external account from their Account Security page for permissions syncing. [#60722](https://github.com/sourcegraph/sourcegraph/pull/60722) | ||
| - Added a "Commits" button to the folders in repos that shows commits for the items in that folder. [#60909](https://github.com/sourcegraph/sourcegraph/pull/60909) | ||
| - The frontend Grafana dashboard has a new Prometheus metric that tracks the rate of requests that Sourcegraph issues to external services. [#61348](https://github.com/sourcegraph/sourcegraph/pull/61348) | ||
| - Added support for the `gitURLType` setting for Gerrit, Sourcegraph now supports cloning from Gerrit via SSH. Note: Not on Cloud yet, like for all code hosts. [#61537](https://github.com/sourcegraph/sourcegraph/pull/61537) |
There was a problem hiding this comment.
Why don't we support SSH on cloud?
There was a problem hiding this comment.
Because we have no way of configuring known_hosts and ssh keypairs in-app right now (that's one of the things on our potential roadmap), it requires a deployment change where you mount those files into gitserver. Since customers on cloud cannot modify the deployment, they cannot do that -> not supported.
https://sourcegraph.com/docs/admin/deploy/kubernetes/configure#ssh-for-cloning
| u, err := url.Parse(cfg.Url) | ||
| if err != nil { | ||
| logger.Warn("Error adding authentication to Gerrit project remote URL.", log.String("url", cfg.Url), log.Error(err)) | ||
| return cfg.Url |
There was a problem hiding this comment.
If this is an error why doesnt this return an error ?
There was a problem hiding this comment.
To be honest, I'm not entirely sure, I followed what we do for all other code host types in this file. Might be good to clean that up though and return proper errors?
There was a problem hiding this comment.
The function where this is called returns string, error anyways, so we could just have these return errors.
There was a problem hiding this comment.
I'll push this as a separate PR to make pinpointing potential issues easier, but agree that handling errors is probably better than logging which customers on Cloud have no insight to!
There was a problem hiding this comment.
| u := *instanceHTTPURL | ||
| u.User = nil | ||
| // Gerrit encodes slashes in IDs, so need to decode them. | ||
| decodedName := strings.ReplaceAll(p.ID, "%2F", "/") |
There was a problem hiding this comment.
Could we use a more general urldecode type method?
There was a problem hiding this comment.
I moved this code over from the clone_url package unmodified - I'm not sure why this was picked and if there's some other things in gerrit that might break (can you have names in gerrit that need to be URL encoded but will not be here?)
I can make the change, but felt slightly risky. WDYT? Worth it?
3 participants
To bring Gerrit support more in line with other code hosts and because a customer recently ran into this limitation, this PR adds support for the SSH flag.
The diff is mostly straightforward, with two things to watch out for:
/a/. This is only used for visuals, but still annoying. So I moved the whole construction into here from the gitserver cloneURL package.Closes https://github.com/sourcegraph/sourcegraph/issues/60597
Test plan
Added tests for the various stages, recreated recorded responses, and tried both HTTP and SSH cloning locally, both worked with out Gerrit instance.