Enable "Test connection" for Perforce code hosts#56697
Conversation
CheckConnection and mark Perforce as able to check connections|
Codenotify: Notifying subscribers in CODENOTIFY files for diff cb63189...3883a38.
|
mrnugget
left a comment
There was a problem hiding this comment.
Can you run this locally to make sure it works as expected? The err == nil tells me you haven't run it locally, so I at least want to make sure it works.
There was a problem hiding this comment.
I'm surprised that we don't have code that already does this. Maybe time to pull this out into the perforce package and allow user and password? https://sourcegraph.sourcegraph.com/github.com/sourcegraph/sourcegraph@2fa7fbba450030d1249935fac6083405351210fa/-/blob/internal/repos/perforce.go?L87-97
There was a problem hiding this comment.
Good idea. We should extract decomposePerforceCloneURL also.
But which perforce package?
-
internal/perforce -
internal/extsvc/perforce -
internal/authz/providers/perforce -
cmd/gitserver/server/perforce
There was a problem hiding this comment.
I'm not sure I understand what the flow is supposed to be: if we can connect to the first depot without a problem, we return, yes? What if we can't connect to the others?
There was a problem hiding this comment.
The approach was confusing; trying to leverage existing methods. Abandoning that approach for a better one.
There was a problem hiding this comment.
Hmmmm, I think we should return the error here, no?
There was a problem hiding this comment.
No, we want to try additional repos ins case the error is with the repo and not with the code host. But that approach is outdated now.
The However, based on your feedback, @mrnugget, I have revised the approach. Instead of leveraging an existing method in the syncer - |
There was a problem hiding this comment.
That comment is quite correct (or at least, it doesn't work in frontend), but importing the syncer from the cmd/gitserver codebase doesn't actually run it in gitserver. If this works, it'll magically do because repo-updater has the p4 cli at the moment as well, but the comment is not 100% accurate.
However, this touches on a great point that it would be ideal to RPC to gitserver to check the perforce status instead.
But this doesn't have to be rushed in the day before branch cut and we can improve it later.
There was a problem hiding this comment.
Hmmm, so internal/repos/perforce.go::CheckConnection is called from cmd/frontend/graphqlbackend/external_service.go::CheckConnection, which is in the frontend, right? So how does that work? Does frontend run in the server container, which does have p4 (and nearly everything else) installed? I'm looking at the architecture diagram.
We should be able to easily convert this call to RPC to gitserver by using P4Exec from gitserver.Client. I'm not sure gitserver.Client will honor any timeouts we add to the context.
I'm not sure I want to switch to that, but this is what it'll look like (tested locally):
func (s PerforceSource) CheckConnection(ctx context.Context) error {
gclient := gitserver.NewClient()
rc, _, err := gclient.P4Exec(ctx, s.config.P4Port, s.config.P4User, s.config.P4Passwd, "users")
if err != nil {
return errors.Wrap(err, "Unable to connect to the Perforce server")
}
rc.Close()
return nil
}
There was a problem hiding this comment.
frontend runs in the frontend container (on k8s) and in the server container if it's single-server image. On k8s you can't guarantee that p4 is installed in the frontend container.
So basically the problem with this code here
is that it should actually send a request to gitserver (from frontend) where then the connection check would be made.
I wouldn't hide that information in the Source.
There was a problem hiding this comment.
All of the other sources than Perforce use HTTP(S) to check the connection with the code host, so they don't need gitserver. It's fine to leave the details in source.CheckConnection; for Perforce we'll make sure to RPC to gitserver for the connection check.
There was a problem hiding this comment.
Created issue #56782 for looking into ListRepos, which is another Source method that may need to also use RPC, although it seems to be ok as-is currently.
Because connecting to Perforce requires using the `p4` CLI binary, which is on `gitserver`, we're using the Perforce VCS Syncer to do the connection check. Add a method to the VCS syncer for Perforce specifically for checking the connection instead of hacking a solution that re-uses IsCloneable, which requires a valid depot. Adding another func won't break composition with VCSSyncer.
10 seconds is somewhat arbitrary, but seems like enough time to allow for various connection issues, whle not being too long for the frontend.
to making an RPC call to gitserver because the call to CheckConnection comes fro mthe frontend, where `p4` is not available.
ba66498 to
3883a38
Compare
* fill out `CheckConnection` and mark Perforce as able to check connections. * Refactor the connection test to not rely on IsCloneable Because connecting to Perforce requires using the `p4` CLI binary, which is on `gitserver`, we're using the Perforce VCS Syncer to do the connection check. Add a method to the VCS syncer for Perforce specifically for checking the connection instead of hacking a solution that re-uses IsCloneable, which requires a valid depot. Adding another func won't break composition with VCSSyncer. * update comment on CheckConnection * add a 10-second timeout for the connection test 10 seconds is somewhat arbitrary, but seems like enough time to allow for various connection issues, whle not being too long for the frontend. * Change from directly using the syncer to making an RPC call to gitserver because the call to CheckConnection comes fro mthe frontend, where `p4` is not available. (cherry picked from commit b1fd4da)
Added CHANGELOG entry for #56697
Enable the "Test connection" button in the code host page UI
Test plan
Click the "Test connection" button in the code host connection UI. A message with an orange border will display, "Checking code host connection..." and then "Code host is reachable" should display with a green border.
