diff --git a/cmd/frontend/graphqlbackend/BUILD.bazel b/cmd/frontend/graphqlbackend/BUILD.bazel index c867ddb4bb887..5ee5abd50942c 100644 --- a/cmd/frontend/graphqlbackend/BUILD.bazel +++ b/cmd/frontend/graphqlbackend/BUILD.bazel @@ -510,6 +510,7 @@ go_test( "//internal/gitserver/gitdomain", "//internal/gitserver/protocol", "//internal/gqlutil", + "//internal/httpcli", "//internal/observation", "//internal/oobmigration", "//internal/ratelimit", diff --git a/cmd/frontend/graphqlbackend/cody_gateway_rate_limit.go b/cmd/frontend/graphqlbackend/cody_gateway_rate_limit.go index 8e5d2e2df598b..1f0897d1e531a 100644 --- a/cmd/frontend/graphqlbackend/cody_gateway_rate_limit.go +++ b/cmd/frontend/graphqlbackend/cody_gateway_rate_limit.go @@ -24,7 +24,8 @@ func (r *siteResolver) CodyGatewayRateLimitStatus(ctx context.Context) (*[]RateL return nil, err } - cgc, ok := codygateway.NewClientFromSiteConfig(httpcli.ExternalDoer) + logger := r.logger.Scoped("CodyGatewayRateLimitStatus") + cgc, ok := codygateway.NewClientFromSiteConfig(httpcli.ExternalDoer(logger)) if !ok { // Not configured for chat/autocomplete. return nil, nil diff --git a/cmd/frontend/graphqlbackend/external_accounts.go b/cmd/frontend/graphqlbackend/external_accounts.go index 8ae251e7f4f1a..9c19697f9b4d7 100644 --- a/cmd/frontend/graphqlbackend/external_accounts.go +++ b/cmd/frontend/graphqlbackend/external_accounts.go @@ -173,7 +173,7 @@ func (r *schemaResolver) AddExternalAccount(ctx context.Context, args *struct { switch args.ServiceType { case extsvc.TypeGerrit: - err := gext.AddGerritExternalAccount(ctx, r.db, a.UID, args.ServiceID, args.AccountDetails) + err := gext.AddGerritExternalAccount(ctx, r.db, r.logger, a.UID, args.ServiceID, args.AccountDetails) if err != nil { return nil, err } diff --git a/cmd/frontend/graphqlbackend/external_accounts_test.go b/cmd/frontend/graphqlbackend/external_accounts_test.go index bb29788d6fd60..130fb70982fcd 100644 --- a/cmd/frontend/graphqlbackend/external_accounts_test.go +++ b/cmd/frontend/graphqlbackend/external_accounts_test.go @@ -180,7 +180,7 @@ func TestExternalAccounts_AddExternalAccount(t *testing.T) { } } - gerrit.MockVerifyAccount = func(_ context.Context, _ *url.URL, _ *gerrit.AccountCredentials) (*gerrit.Account, error) { + gerrit.MockVerifyAccount = func(_ context.Context, _ log.Logger, _ *url.URL, _ *gerrit.AccountCredentials) (*gerrit.Account, error) { return &gerrit.Account{ ID: 1234, Username: "alice", diff --git a/cmd/frontend/graphqlbackend/external_service.go b/cmd/frontend/graphqlbackend/external_service.go index d4d61a8d8eb49..4966fbfb90853 100644 --- a/cmd/frontend/graphqlbackend/external_service.go +++ b/cmd/frontend/graphqlbackend/external_service.go @@ -311,12 +311,14 @@ func (r *externalServiceResolver) CheckConnection(ctx context.Context) (*externa ctx, cancel := context.WithTimeout(ctx, 15*time.Second) defer cancel() + logger := r.logger.Scoped("externalServiceResolver.CheckConnection") + source, err := repos.NewSource( ctx, - log.Scoped("externalServiceResolver.CheckConnection"), + logger, r.db, r.externalService, - httpcli.ExternalClientFactory, + httpcli.ExternalClientFactory(logger.Scoped("httpclient")), gitserver.NewClient("graphql.check-connection"), ) if err != nil { diff --git a/cmd/frontend/graphqlbackend/guardrails_test.go b/cmd/frontend/graphqlbackend/guardrails_test.go index 5587594947ce2..e999514a15452 100644 --- a/cmd/frontend/graphqlbackend/guardrails_test.go +++ b/cmd/frontend/graphqlbackend/guardrails_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/graph-gophers/graphql-go" + "github.com/sourcegraph/log" "github.com/stretchr/testify/require" "github.com/sourcegraph/sourcegraph/cmd/frontend/enterprise" @@ -20,6 +21,7 @@ import ( "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/database/dbmocks" "github.com/sourcegraph/sourcegraph/internal/gitserver" + "github.com/sourcegraph/sourcegraph/internal/httpcli" "github.com/sourcegraph/sourcegraph/internal/observation" "github.com/sourcegraph/sourcegraph/lib/errors" "github.com/sourcegraph/sourcegraph/lib/pointers" @@ -194,7 +196,9 @@ func makeGatewayEndpoint(t *testing.T) string { func TestSnippetAttributionReactsToSiteConfigChanges(t *testing.T) { // Use a regular HTTP client as default external doer cannot hit localhost. - guardrails.MockHttpClient = http.DefaultClient + guardrails.MockHttpClient = func(_ log.Logger) httpcli.Doer { + return http.DefaultClient + } t.Cleanup(func() { guardrails.MockHttpClient = nil }) // Starting attribution configuration has no endpoints to use. noAttributionConfigured := schema.SiteConfiguration{ diff --git a/cmd/frontend/graphqlbackend/survey_response.go b/cmd/frontend/graphqlbackend/survey_response.go index 3c7ba9f4dd704..30cd7dada35e1 100644 --- a/cmd/frontend/graphqlbackend/survey_response.go +++ b/cmd/frontend/graphqlbackend/survey_response.go @@ -117,7 +117,7 @@ func (r *schemaResolver) SubmitSurvey(ctx context.Context, args *struct { } // Submit form to HubSpot - if err := hubspotutil.Client().SubmitForm(hubspotutil.SurveyFormID, &surveySubmissionForHubSpot{ + if err := hubspotutil.Client(r.logger).SubmitForm(hubspotutil.SurveyFormID, &surveySubmissionForHubSpot{ Email: email, Score: args.Input.Score, OtherUseCase: args.Input.OtherUseCase, @@ -181,7 +181,7 @@ func (r *schemaResolver) SubmitHappinessFeedback(ctx context.Context, args *stru } // Submit form to HubSpot - if err := hubspotutil.Client().SubmitForm(hubspotutil.HappinessFeedbackFormID, &data); err != nil { + if err := hubspotutil.Client(r.logger).SubmitForm(hubspotutil.HappinessFeedbackFormID, &data); err != nil { // Log an error, but don't return one if the only failure was in submitting feedback results to HubSpot. log15.Error("Unable to submit happiness feedback results to Sourcegraph remote", "error", err) } diff --git a/cmd/frontend/graphqlbackend/user_usage_stats.go b/cmd/frontend/graphqlbackend/user_usage_stats.go index 656e7d0b7ec04..19567f8e6f746 100644 --- a/cmd/frontend/graphqlbackend/user_usage_stats.go +++ b/cmd/frontend/graphqlbackend/user_usage_stats.go @@ -169,11 +169,11 @@ func (r *schemaResolver) LogEvents(ctx context.Context, args *EventBatch) (*Empt } } - hubspotutil.SyncUser(userPrimaryEmail, hubspotutil.CodyClientInstalledEventID, &hubspot.ContactProperties{ + hubspotutil.SyncUser(r.logger, userPrimaryEmail, hubspotutil.CodyClientInstalledEventID, &hubspot.ContactProperties{ DatabaseID: userID, }) - hubspotutil.SyncUserWithV3Event(userPrimaryEmail, hubspotutil.CodyClientInstalledV3EventID, + hubspotutil.SyncUserWithV3Event(r.logger, userPrimaryEmail, hubspotutil.CodyClientInstalledV3EventID, &hubspot.ContactProperties{ DatabaseID: userID, }, diff --git a/cmd/frontend/hubspot/BUILD.bazel b/cmd/frontend/hubspot/BUILD.bazel index 2788ec0c6c53c..63edda6e714e0 100644 --- a/cmd/frontend/hubspot/BUILD.bazel +++ b/cmd/frontend/hubspot/BUILD.bazel @@ -14,6 +14,7 @@ go_library( "//internal/httpcli", "//lib/errors", "@com_github_google_go_querystring//query", + "@com_github_sourcegraph_log//:log", "@org_uber_go_atomic//:atomic", ], ) diff --git a/cmd/frontend/hubspot/hubspot.go b/cmd/frontend/hubspot/hubspot.go index 5b4ddc802ed70..ab7a686221df1 100644 --- a/cmd/frontend/hubspot/hubspot.go +++ b/cmd/frontend/hubspot/hubspot.go @@ -12,6 +12,7 @@ import ( "time" "github.com/google/go-querystring/query" + "github.com/sourcegraph/log" "go.uber.org/atomic" "github.com/sourcegraph/sourcegraph/internal/httpcli" @@ -25,13 +26,15 @@ type Client struct { lastPing atomic.Time lastPingResult atomic.Error + logger log.Logger } // New returns a new HubSpot client using the given Portal ID. -func New(portalID, accessToken string) *Client { +func New(portalID, accessToken string, logger log.Logger) *Client { return &Client{ portalID: portalID, accessToken: accessToken, + logger: logger.Scoped("HubSpotClient"), } } @@ -61,7 +64,7 @@ func (c *Client) postForm(methodName string, baseURL *url.URL, suffix string, bo req.Header.Set("Content-Type", "application/x-www-form-urlencoded") setAccessTokenAuthorizationHeader(req, c.accessToken) - resp, err := httpcli.ExternalDoer.Do(req) + resp, err := httpcli.ExternalDoer(c.logger).Do(req) if err != nil { return wrapError(methodName, err) } @@ -95,7 +98,7 @@ func (c *Client) postJSON(methodName string, baseURL *url.URL, reqPayload, respP req.Header.Set("Content-Type", "application/json") setAccessTokenAuthorizationHeader(req, c.accessToken) - resp, err := httpcli.ExternalDoer.Do(req.WithContext(ctx)) + resp, err := httpcli.ExternalDoer(c.logger).Do(req.WithContext(ctx)) if err != nil { return wrapError(methodName, err) } @@ -129,7 +132,7 @@ func (c *Client) get(ctx context.Context, methodName string, baseURL *url.URL, s ctx, cancel := context.WithTimeout(req.Context(), time.Minute) defer cancel() - resp, err := httpcli.ExternalDoer.Do(req.WithContext(ctx)) + resp, err := httpcli.ExternalDoer(c.logger).Do(req.WithContext(ctx)) if err != nil { return wrapError(methodName, err) } diff --git a/cmd/frontend/hubspot/hubspotutil/BUILD.bazel b/cmd/frontend/hubspot/hubspotutil/BUILD.bazel index 6803f6c4b7d86..f599257b4e682 100644 --- a/cmd/frontend/hubspot/hubspotutil/BUILD.bazel +++ b/cmd/frontend/hubspot/hubspotutil/BUILD.bazel @@ -11,5 +11,6 @@ go_library( "//internal/env", "//lib/errors", "@com_github_inconshreveable_log15//:log15", + "@com_github_sourcegraph_log//:log", ], ) diff --git a/cmd/frontend/hubspot/hubspotutil/hubspotutil.go b/cmd/frontend/hubspot/hubspotutil/hubspotutil.go index c289ca9c97208..e43a0b10c5b5c 100644 --- a/cmd/frontend/hubspot/hubspotutil/hubspotutil.go +++ b/cmd/frontend/hubspot/hubspotutil/hubspotutil.go @@ -5,6 +5,7 @@ import ( "log" //nolint:logging // TODO move all logging to sourcegraph/log "github.com/inconshreveable/log15" //nolint:logging // TODO move all logging to sourcegraph/log + logger "github.com/sourcegraph/log" "github.com/sourcegraph/sourcegraph/cmd/frontend/hubspot" "github.com/sourcegraph/sourcegraph/internal/dotcom" @@ -52,21 +53,17 @@ func HasAPIKey() bool { return HubSpotAccessToken != "" } -func init() { +// Client returns a hubspot client +func Client(l logger.Logger) *hubspot.Client { // The HubSpot access token will only be available in the production sourcegraph.com environment. // Not having this access token only restricts certain requests (e.g. GET requests to the Contacts API), // while others (e.g. POST requests to the Forms API) will still go through. - client = hubspot.New("2762526", HubSpotAccessToken) -} - -// Client returns a hubspot client -func Client() *hubspot.Client { - return client + return hubspot.New("2762526", HubSpotAccessToken, l.Scoped("HubSpotClient")) } // SyncUser handles creating or syncing a user profile in HubSpot, and if provided, // logs a user event. -func SyncUser(email, eventID string, contactParams *hubspot.ContactProperties) { +func SyncUser(l logger.Logger, email, eventID string, contactParams *hubspot.ContactProperties) { defer func() { if err := recover(); err != nil { log.Printf("panic in tracking.SyncUser: %s", err) @@ -79,7 +76,7 @@ func SyncUser(email, eventID string, contactParams *hubspot.ContactProperties) { // Update or create user contact information in HubSpot, and we want to sync the // contact independent of the request lifecycle. - err := syncHubSpotContact(context.Background(), email, eventID, contactParams) + err := syncHubSpotContact(context.Background(), l, email, eventID, contactParams) if err != nil { log15.Warn("syncHubSpotContact: failed to create or update HubSpot contact", "source", "HubSpot", "eventID", eventID, "error", err) } @@ -87,7 +84,7 @@ func SyncUser(email, eventID string, contactParams *hubspot.ContactProperties) { // SyncUserWithV3Event handles creating or syncing a user profile in HubSpot, and if provided, // logs a V3 custom event along with the event params. -func SyncUserWithV3Event(email, eventName string, contactParams *hubspot.ContactProperties, eventProperties any) { +func SyncUserWithV3Event(l logger.Logger, email, eventName string, contactParams *hubspot.ContactProperties, eventProperties any) { defer func() { if err := recover(); err != nil { log.Printf("panic in tracking.SyncUserWithV3Event: %s", err) @@ -101,14 +98,14 @@ func SyncUserWithV3Event(email, eventName string, contactParams *hubspot.Contact // Update or create user contact information in HubSpot, and we want to sync the // contact independent of the request lifecycle. - err := syncHubSpotContact(context.Background(), email, "", contactParams) + err := syncHubSpotContact(context.Background(), l, email, "", contactParams) if err != nil { log15.Warn("syncHubSpotContact: failed to create or update HubSpot contact", "source", "HubSpot", "eventName", eventName, "error", err) } // Log the V3 event if eventName != "" { - c := Client() + c := Client(l) err = c.LogV3Event(email, eventName, eventProperties) if err != nil { log.Printf("LOGV3Event: failed to event %s", err) @@ -117,7 +114,7 @@ func SyncUserWithV3Event(email, eventName string, contactParams *hubspot.Contact } } -func syncHubSpotContact(ctx context.Context, email, eventID string, contactParams *hubspot.ContactProperties) error { +func syncHubSpotContact(ctx context.Context, l logger.Logger, email, eventID string, contactParams *hubspot.ContactProperties) error { if email == "" { return errors.New("user must have a valid email address") } @@ -128,7 +125,7 @@ func syncHubSpotContact(ctx context.Context, email, eventID string, contactParam } contactParams.UserID = email - c := Client() + c := Client(l) // Create or update the contact _, err := c.CreateOrUpdateContact(email, contactParams) diff --git a/cmd/frontend/internal/app/ui/handlers.go b/cmd/frontend/internal/app/ui/handlers.go index c74ae59669335..33b9eb2dd24f3 100644 --- a/cmd/frontend/internal/app/ui/handlers.go +++ b/cmd/frontend/internal/app/ui/handlers.go @@ -563,68 +563,72 @@ func searchBadgeHandler() *httputil.ReverseProxy { } } -func servePingFromSelfHosted(w http.ResponseWriter, r *http.Request) error { - // CORS to allow request from anywhere - u, err := url.Parse(r.Referer()) - if err != nil { - return err - } - w.Header().Add("Access-Control-Allow-Origin", u.Host) - w.Header().Add("Access-Control-Allow-Credentials", "true") - if r.Method == http.MethodOptions { - // CORS preflight request, respond 204 and allow origin header - w.WriteHeader(http.StatusNoContent) - return nil - } - email := r.URL.Query().Get("email") - tosAccepted := r.URL.Query().Get("tos_accepted") +func servePingFromSelfHosted(l log.Logger) func(w http.ResponseWriter, r *http.Request) error { + l = l.Scoped("servePingFromSelfHosted") - getCookie := func(name string) string { - c, err := r.Cookie(name) - if err != nil || c == nil { - return "" + return func(w http.ResponseWriter, r *http.Request) error { + // CORS to allow request from anywhere + u, err := url.Parse(r.Referer()) + if err != nil { + return err } - return c.Value - } + w.Header().Add("Access-Control-Allow-Origin", u.Host) + w.Header().Add("Access-Control-Allow-Credentials", "true") + if r.Method == http.MethodOptions { + // CORS preflight request, respond 204 and allow origin header + w.WriteHeader(http.StatusNoContent) + return nil + } + email := r.URL.Query().Get("email") + tosAccepted := r.URL.Query().Get("tos_accepted") - anonymousUserId, _ := cookie.AnonymousUID(r) - - hubspotutil.SyncUser(email, hubspotutil.SelfHostedSiteInitEventID, &hubspot.ContactProperties{ - IsServerAdmin: true, - AnonymousUserID: anonymousUserId, - FirstSourceURL: getCookie("first_page_seen_url"), - LastSourceURL: getCookie("last_page_seen_url"), - LastPageSeenShort: getCookie("last_page_seen_url_short"), - LastPageSeenMid: getCookie("last_page_seen_url_mid"), - LastPageSeenLong: getCookie("last_page_seen_url_long"), - MostRecentReferrerUrl: getCookie("most_recent_referrer_url"), - MostRecentReferrerUrlShort: getCookie("most_recent_referrer_url_short"), - MostRecentReferrerUrlLong: getCookie("most_recent_referrer_url_long"), - SignupSessionSourceURL: getCookie("sourcegraphSignupSourceUrl"), - SignupSessionReferrer: getCookie("sourcegraphSignupReferrer"), - SessionUTMCampaign: getCookie("utm_campaign"), - SessionUTMSource: getCookie("utm_source"), - SessionUTMMedium: getCookie("utm_medium"), - SessionUTMContent: getCookie("utm_content"), - SessionUTMTerm: getCookie("utm_term"), - UtmCampaignShort: getCookie("utm_campaign_short"), - UtmCampaignMid: getCookie("utm_campaign_mid"), - UtmCampaignLong: getCookie("utm_campaign_long"), - UtmSourceShort: getCookie("utm_source_short"), - UtmSourceMid: getCookie("utm_source_mid"), - UtmSourceLong: getCookie("utm_source_long"), - UtmMediumShort: getCookie("utm_medium_short"), - UtmMediumMid: getCookie("utm_medium_mid"), - UtmMediumLong: getCookie("utm_medium_long"), - UtmContentShort: getCookie("utm_content_short"), - UtmContentMid: getCookie("utm_content_mid"), - UtmContentLong: getCookie("utm_content_long"), - UtmTermShort: getCookie("utm_term_short"), - UtmTermMid: getCookie("utm_term_mid"), - UtmTermLong: getCookie("utm_term_long"), - GoogleClickID: getCookie("gclid"), - MicrosoftClickID: getCookie("msclkid"), - HasAgreedToToS: tosAccepted == "true", - }) - return nil + getCookie := func(name string) string { + c, err := r.Cookie(name) + if err != nil || c == nil { + return "" + } + return c.Value + } + + anonymousUserId, _ := cookie.AnonymousUID(r) + + hubspotutil.SyncUser(l, email, hubspotutil.SelfHostedSiteInitEventID, &hubspot.ContactProperties{ + IsServerAdmin: true, + AnonymousUserID: anonymousUserId, + FirstSourceURL: getCookie("first_page_seen_url"), + LastSourceURL: getCookie("last_page_seen_url"), + LastPageSeenShort: getCookie("last_page_seen_url_short"), + LastPageSeenMid: getCookie("last_page_seen_url_mid"), + LastPageSeenLong: getCookie("last_page_seen_url_long"), + MostRecentReferrerUrl: getCookie("most_recent_referrer_url"), + MostRecentReferrerUrlShort: getCookie("most_recent_referrer_url_short"), + MostRecentReferrerUrlLong: getCookie("most_recent_referrer_url_long"), + SignupSessionSourceURL: getCookie("sourcegraphSignupSourceUrl"), + SignupSessionReferrer: getCookie("sourcegraphSignupReferrer"), + SessionUTMCampaign: getCookie("utm_campaign"), + SessionUTMSource: getCookie("utm_source"), + SessionUTMMedium: getCookie("utm_medium"), + SessionUTMContent: getCookie("utm_content"), + SessionUTMTerm: getCookie("utm_term"), + UtmCampaignShort: getCookie("utm_campaign_short"), + UtmCampaignMid: getCookie("utm_campaign_mid"), + UtmCampaignLong: getCookie("utm_campaign_long"), + UtmSourceShort: getCookie("utm_source_short"), + UtmSourceMid: getCookie("utm_source_mid"), + UtmSourceLong: getCookie("utm_source_long"), + UtmMediumShort: getCookie("utm_medium_short"), + UtmMediumMid: getCookie("utm_medium_mid"), + UtmMediumLong: getCookie("utm_medium_long"), + UtmContentShort: getCookie("utm_content_short"), + UtmContentMid: getCookie("utm_content_mid"), + UtmContentLong: getCookie("utm_content_long"), + UtmTermShort: getCookie("utm_term_short"), + UtmTermMid: getCookie("utm_term_mid"), + UtmTermLong: getCookie("utm_term_long"), + GoogleClickID: getCookie("gclid"), + MicrosoftClickID: getCookie("msclkid"), + HasAgreedToToS: tosAccepted == "true", + }) + return nil + } } diff --git a/cmd/frontend/internal/app/ui/router.go b/cmd/frontend/internal/app/ui/router.go index 970bd48790258..18282adcc9160 100644 --- a/cmd/frontend/internal/app/ui/router.go +++ b/cmd/frontend/internal/app/ui/router.go @@ -113,7 +113,7 @@ func InitRouter(db database.DB, configurationServer *conf.Server) { r.Path("/").Methods(http.MethodGet, http.MethodHead).Name(routeHome).Handler(handler(db, configurationServer, serveHome(db, configurationServer))) r.Path("/sign-in").Methods(http.MethodGet, http.MethodHead).Name(uirouter.RouteSignIn).Handler(handler(db, configurationServer, serveSignIn(db, configurationServer))) - r.Path("/ping-from-self-hosted").Methods("GET", "OPTIONS").Name(uirouter.RoutePingFromSelfHosted).Handler(handler(db, configurationServer, servePingFromSelfHosted)) + r.Path("/ping-from-self-hosted").Methods("GET", "OPTIONS").Name(uirouter.RoutePingFromSelfHosted).Handler(handler(db, configurationServer, servePingFromSelfHosted(logger))) ghAppRouter := r.PathPrefix("/githubapp/").Subrouter() githubapp.SetupGitHubAppRoutes(ghAppRouter, db) diff --git a/cmd/frontend/internal/auth/azureoauth/login.go b/cmd/frontend/internal/auth/azureoauth/login.go index 16e174c777fa6..54f77b1d41bba 100644 --- a/cmd/frontend/internal/auth/azureoauth/login.go +++ b/cmd/frontend/internal/auth/azureoauth/login.go @@ -7,10 +7,11 @@ import ( "github.com/dghubble/gologin/v2" oauth2Login "github.com/dghubble/gologin/v2/oauth2" "github.com/sourcegraph/log" + "golang.org/x/oauth2" + "github.com/sourcegraph/sourcegraph/internal/extsvc/auth" "github.com/sourcegraph/sourcegraph/internal/extsvc/azuredevops" "github.com/sourcegraph/sourcegraph/lib/errors" - "golang.org/x/oauth2" ) func loginHandler(c oauth2.Config) http.Handler { @@ -33,6 +34,7 @@ func azureDevOpsHandler(logger log.Logger, config *oauth2.Config, success, failu azuredevops.VisualStudioAppURL, &auth.OAuthBearerToken{Token: token.AccessToken}, nil, + logger, ) if err != nil { logger.Error("failed to create azuredevops.Client", log.String("error", err.Error())) diff --git a/cmd/frontend/internal/auth/azureoauth/session.go b/cmd/frontend/internal/auth/azureoauth/session.go index 44f4c0d578828..6ce845cad7e5c 100644 --- a/cmd/frontend/internal/auth/azureoauth/session.go +++ b/cmd/frontend/internal/auth/azureoauth/session.go @@ -41,7 +41,7 @@ func (s *sessionIssuerHelper) GetOrCreateUser(ctx context.Context, token *oauth2 return false, nil, "failed to read Azure DevOps Profile from oauth2 callback request", errors.Wrap(err, "azureoauth.GetOrCreateUser: failed to read user from context of callback request") } - if allow, err := s.verifyAllowOrgs(ctx, user, token, httpcli.ExternalDoer); err != nil { + if allow, err := s.verifyAllowOrgs(ctx, user, token, httpcli.ExternalDoer(s.logger)); err != nil { return false, nil, "error in verifying authorized user organizations", err } else if !allow { msg := "User does not belong to any org from the allowed list of organizations. Please contact your site admin." @@ -128,6 +128,7 @@ func (s *sessionIssuerHelper) verifyAllowOrgs(ctx context.Context, profile *azur Token: token.AccessToken, }, doer, + s.logger, ) if err != nil { return false, errors.Wrap(err, "failed to create client for listing organizations of user") diff --git a/cmd/frontend/internal/auth/bitbucketcloudoauth/session.go b/cmd/frontend/internal/auth/bitbucketcloudoauth/session.go index 35304501a2f2a..db9e79051a9e8 100644 --- a/cmd/frontend/internal/auth/bitbucketcloudoauth/session.go +++ b/cmd/frontend/internal/auth/bitbucketcloudoauth/session.go @@ -56,7 +56,7 @@ func (s *sessionIssuerHelper) GetOrCreateUser(ctx context.Context, token *oauth2 conf := &schema.BitbucketCloudConnection{ Url: s.baseURL.String(), } - client, err = bitbucketcloud.NewClient(s.baseURL.String(), conf, nil) + client, err = bitbucketcloud.NewClient(s.baseURL.String(), conf, nil, s.logger) if err != nil { return false, nil, "Could not initialize Bitbucket Cloud client", err } @@ -113,7 +113,7 @@ func (s *sessionIssuerHelper) GetOrCreateUser(ctx context.Context, token *oauth2 CreateIfNotExist: attempt.createIfNotExist, }) if err == nil { - go hubspotutil.SyncUser(attempt.email, hubspotutil.SignupEventID, hubSpotProps) + go hubspotutil.SyncUser(s.logger, attempt.email, hubspotutil.SignupEventID, hubSpotProps) return newUserCreated, actor.FromUser(userID), "", nil } if i == 0 { diff --git a/cmd/frontend/internal/auth/bitbucketcloudoauth/session_test.go b/cmd/frontend/internal/auth/bitbucketcloudoauth/session_test.go index 622197f5b0787..d3065b913fddc 100644 --- a/cmd/frontend/internal/auth/bitbucketcloudoauth/session_test.go +++ b/cmd/frontend/internal/auth/bitbucketcloudoauth/session_test.go @@ -196,7 +196,7 @@ func TestSessionIssuerHelper_GetOrCreateUser(t *testing.T) { Url: server.URL, ApiURL: server.URL, } - bbClient, err := bitbucketcloud.NewClient(server.URL, conf, httpcli.TestExternalDoer) + bbClient, err := bitbucketcloud.NewClient(server.URL, conf, httpcli.TestExternalDoer, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -278,7 +278,7 @@ func TestSessionIssuerHelper_SignupMatchesSecondaryAccount(t *testing.T) { Url: server.URL, ApiURL: server.URL, } - bbClient, err := bitbucketcloud.NewClient(server.URL, conf, httpcli.TestExternalDoer) + bbClient, err := bitbucketcloud.NewClient(server.URL, conf, httpcli.TestExternalDoer, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -350,7 +350,7 @@ func TestGetOrCreateUser_NoPanicOnEmailSlice(t *testing.T) { Url: server.URL, ApiURL: server.URL, } - bbClient, err := bitbucketcloud.NewClient(server.URL, conf, httpcli.TestExternalDoer) + bbClient, err := bitbucketcloud.NewClient(server.URL, conf, httpcli.TestExternalDoer, logtest.Scoped(t)) if err != nil { t.Fatalf("Failed to create Bitbucket Cloud client: %v", err) } diff --git a/cmd/frontend/internal/auth/githuboauth/session.go b/cmd/frontend/internal/auth/githuboauth/session.go index a072a122e133f..52bc4cc9fef64 100644 --- a/cmd/frontend/internal/auth/githuboauth/session.go +++ b/cmd/frontend/internal/auth/githuboauth/session.go @@ -159,7 +159,7 @@ func (s *sessionIssuerHelper) GetOrCreateUser(ctx context.Context, token *oauth2 CreateIfNotExist: attempt.createIfNotExist, }) if err == nil { - go hubspotutil.SyncUser(attempt.email, hubspotutil.SignupEventID, hubSpotProps) + go hubspotutil.SyncUser(s.logger, attempt.email, hubspotutil.SignupEventID, hubSpotProps) return newUserCreated, actor.FromUser(userID), "", nil // success } lastSafeErrMsg, lastErr = safeErrMsg, err diff --git a/cmd/frontend/internal/auth/gitlaboauth/login.go b/cmd/frontend/internal/auth/gitlaboauth/login.go index 4076f4c9c94a6..d28f2506d4038 100644 --- a/cmd/frontend/internal/auth/gitlaboauth/login.go +++ b/cmd/frontend/internal/auth/gitlaboauth/login.go @@ -63,13 +63,13 @@ func LoginHandler(config *oauth2.Config, failure http.Handler) http.Handler { return oauth2Login.LoginHandler(config, failure) } -func CallbackHandler(config *oauth2.Config, success, failure http.Handler) http.Handler { - success = gitlabHandler(config, success, failure) +func CallbackHandler(config *oauth2.Config, logger log.Logger, success, failure http.Handler) http.Handler { + success = gitlabHandler(config, logger, success, failure) return oauth2Login.CallbackHandler(config, success, failure) } -func gitlabHandler(config *oauth2.Config, success, failure http.Handler) http.Handler { - logger := log.Scoped("GitlabOAuthHandler") +func gitlabHandler(config *oauth2.Config, logger log.Logger, success, failure http.Handler) http.Handler { + logger = logger.Scoped("GitlabOAuthHandler") if failure == nil { failure = gologin.DefaultFailureHandler @@ -83,7 +83,7 @@ func gitlabHandler(config *oauth2.Config, success, failure http.Handler) http.Ha return } - gitlabClient, err := gitlabClientFromAuthURL(config.Endpoint.AuthURL, token.AccessToken) + gitlabClient, err := gitlabClientFromAuthURL(config.Endpoint.AuthURL, token.AccessToken, logger) if err != nil { ctx = gologin.WithError(ctx, errors.Errorf("could not parse AuthURL %s", config.Endpoint.AuthURL)) failure.ServeHTTP(w, req.WithContext(ctx)) @@ -119,7 +119,7 @@ func validateResponse(user *gitlab.AuthUser, err error) error { return nil } -func gitlabClientFromAuthURL(authURL, oauthToken string) (*gitlab.Client, error) { +func gitlabClientFromAuthURL(authURL, oauthToken string, logger log.Logger) (*gitlab.Client, error) { baseURL, err := url.Parse(authURL) if err != nil { return nil, err @@ -127,5 +127,5 @@ func gitlabClientFromAuthURL(authURL, oauthToken string) (*gitlab.Client, error) baseURL.Path = "" baseURL.RawQuery = "" baseURL.Fragment = "" - return gitlab.NewClientProvider(extsvc.URNGitLabOAuth, baseURL, nil).GetOAuthClient(oauthToken), nil + return gitlab.NewClientProvider(extsvc.URNGitLabOAuth, baseURL, nil, logger).GetOAuthClient(oauthToken), nil } diff --git a/cmd/frontend/internal/auth/gitlaboauth/provider.go b/cmd/frontend/internal/auth/gitlaboauth/provider.go index eb52e6cce0755..5abadac0e1ac0 100644 --- a/cmd/frontend/internal/auth/gitlaboauth/provider.go +++ b/cmd/frontend/internal/auth/gitlaboauth/provider.go @@ -30,6 +30,8 @@ func parseProvider(logger log.Logger, db database.DB, callbackURL string, p *sch } codeHost := extsvc.NewCodeHost(parsedURL, extsvc.TypeGitLab) + logger = logger.Scoped("GitLabOAuth") + return oauth.NewProvider(oauth.ProviderOp{ AuthPrefix: authPrefix, OAuth2Config: func() oauth2.Config { @@ -59,6 +61,7 @@ func parseProvider(logger log.Logger, db database.DB, callbackURL string, p *sch Callback: func(oauth2Cfg oauth2.Config) http.Handler { return CallbackHandler( &oauth2Cfg, + logger, oauth.SessionIssuer(logger, db, &sessionIssuerHelper{ logger: logger.Scoped("sessionIssuerHelper"), db: db, diff --git a/cmd/frontend/internal/auth/gitlaboauth/session.go b/cmd/frontend/internal/auth/gitlaboauth/session.go index 8b9e1c2f32cdd..b682d30950d67 100644 --- a/cmd/frontend/internal/auth/gitlaboauth/session.go +++ b/cmd/frontend/internal/auth/gitlaboauth/session.go @@ -73,7 +73,7 @@ func (s *sessionIssuerHelper) GetOrCreateUser(ctx context.Context, token *oauth2 return false, nil, fmt.Sprintf("Error normalizing the username %q. See https://sourcegraph.com/docs/admin/auth/#username-normalization.", login), err } - provider := gitlab.NewClientProvider(extsvc.URNGitLabOAuth, s.BaseURL, nil) + provider := gitlab.NewClientProvider(extsvc.URNGitLabOAuth, s.BaseURL, nil, s.logger) glClient := provider.GetOAuthClient(token.AccessToken) // 🚨 SECURITY: Ensure that the user is part of one of the allowed groups or subgroups when the allowGroups option is set. @@ -124,7 +124,7 @@ func (s *sessionIssuerHelper) GetOrCreateUser(ctx context.Context, token *oauth2 // There is no need to send record if we know email is empty as it's a primary property if gUser.Email != "" { - go hubspotutil.SyncUser(gUser.Email, hubspotutil.SignupEventID, hubSpotProps) + go hubspotutil.SyncUser(s.logger, gUser.Email, hubspotutil.SignupEventID, hubSpotProps) } return newUserCreated, actor.FromUser(userID), "", nil diff --git a/cmd/frontend/internal/auth/oauth/middleware.go b/cmd/frontend/internal/auth/oauth/middleware.go index 3566ed2579b18..e1cec855df3b8 100644 --- a/cmd/frontend/internal/auth/oauth/middleware.go +++ b/cmd/frontend/internal/auth/oauth/middleware.go @@ -125,11 +125,12 @@ func newOAuthFlowHandler(serviceType string) http.Handler { // golang.org/x/oauth2 package will use our http client which is configured // with proxy and TLS settings/etc. func withOAuthExternalClient(r *http.Request) *http.Request { - client := httpcli.ExternalClient + logger := log.Scoped("oauth_external.transport") + client := httpcli.ExternalClient(logger) if traceLogEnabled { loggingClient := *client loggingClient.Transport = &loggingRoundTripper{ - log: log.Scoped("oauth_external.transport"), + log: logger.Scoped("customLoggingRoundTripper"), underlying: client.Transport, } client = &loggingClient diff --git a/cmd/frontend/internal/auth/openidconnect/config.go b/cmd/frontend/internal/auth/openidconnect/config.go index b7c83eb5bac0a..a0cc04b50aed9 100644 --- a/cmd/frontend/internal/auth/openidconnect/config.go +++ b/cmd/frontend/internal/auth/openidconnect/config.go @@ -99,8 +99,9 @@ func getProviders() []providers.Provider { cfgs = append(cfgs, p.Openidconnect) } ps := make([]providers.Provider, 0, len(cfgs)) + logger := log.Scoped("openidconnectProvider") for _, cfg := range cfgs { - ps = append(ps, NewProvider(*cfg, authPrefix, path.Join(auth.AuthURLPrefix, "callback"), httpcli.ExternalClient)) + ps = append(ps, NewProvider(*cfg, authPrefix, path.Join(auth.AuthURLPrefix, "callback"), httpcli.ExternalClient(logger))) } return ps } diff --git a/cmd/frontend/internal/auth/openidconnect/user.go b/cmd/frontend/internal/auth/openidconnect/user.go index da2b5fa9ccef7..7ca4b112f5147 100644 --- a/cmd/frontend/internal/auth/openidconnect/user.go +++ b/cmd/frontend/internal/auth/openidconnect/user.go @@ -108,7 +108,7 @@ func getOrCreateUser( if err != nil { return false, nil, safeErrMsg, err } - go hubspotutil.SyncUser(email, hubspotutil.SignupEventID, hubSpotProps) + go hubspotutil.SyncUser(logger, email, hubspotutil.SignupEventID, hubSpotProps) return newUserCreated, actor.FromUser(userID), "", nil } diff --git a/cmd/frontend/internal/auth/saml/config.go b/cmd/frontend/internal/auth/saml/config.go index 19d5908b87a4e..560abd8f0db7c 100644 --- a/cmd/frontend/internal/auth/saml/config.go +++ b/cmd/frontend/internal/auth/saml/config.go @@ -108,8 +108,9 @@ func getProviders() []*provider { } multiple := len(cfgs) >= 2 ps := make([]*provider, 0, len(cfgs)) + logger := log.Scoped("SAMLAuthProvider") for _, cfg := range cfgs { - p := &provider{config: *cfg, multiple: multiple, httpClient: httpcli.ExternalClient} + p := &provider{config: *cfg, multiple: multiple, httpClient: httpcli.ExternalClient(logger)} ps = append(ps, p) } return ps diff --git a/cmd/frontend/internal/auth/sourcegraphoperator/config.go b/cmd/frontend/internal/auth/sourcegraphoperator/config.go index 29e4c48a8e36d..2708dd692c377 100644 --- a/cmd/frontend/internal/auth/sourcegraphoperator/config.go +++ b/cmd/frontend/internal/auth/sourcegraphoperator/config.go @@ -1,6 +1,8 @@ package sourcegraphoperator import ( + "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/openidconnect" "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/providers" "github.com/sourcegraph/sourcegraph/internal/auth" @@ -35,7 +37,7 @@ func Init() { conf.ContributeValidator(validateConfig) - p := NewProvider(*cloudSiteConfig.AuthProviders.SourcegraphOperator, httpcli.ExternalClient) + p := NewProvider(*cloudSiteConfig.AuthProviders.SourcegraphOperator, httpcli.ExternalClient(log.Scoped("SourcegraphOperator"))) providers.Update(auth.SourcegraphOperatorProviderType, []providers.Provider{p}) } diff --git a/cmd/frontend/internal/auth/userpasswd/handlers.go b/cmd/frontend/internal/auth/userpasswd/handlers.go index 42c8e81a0e911..7981cd430d638 100644 --- a/cmd/frontend/internal/auth/userpasswd/handlers.go +++ b/cmd/frontend/internal/auth/userpasswd/handlers.go @@ -143,7 +143,7 @@ func handleSignUp(logger log.Logger, db database.DB, eventRecorder *telemetry.Ev return c.Value } - go hubspotutil.SyncUser(creds.Email, hubspotutil.SignupEventID, &hubspot.ContactProperties{ + go hubspotutil.SyncUser(logger, creds.Email, hubspotutil.SignupEventID, &hubspot.ContactProperties{ DatabaseID: usr.ID, AnonymousUserID: creds.AnonymousUserID, FirstSourceURL: creds.FirstSourceURL, diff --git a/cmd/frontend/internal/codemonitors/resolvers/resolvers.go b/cmd/frontend/internal/codemonitors/resolvers/resolvers.go index a895e81ba081c..5c55c7ec7a394 100644 --- a/cmd/frontend/internal/codemonitors/resolvers/resolvers.go +++ b/cmd/frontend/internal/codemonitors/resolvers/resolvers.go @@ -412,7 +412,9 @@ func (r *Resolver) TriggerTestWebhookAction(ctx context.Context, args *graphqlba return nil, err } - if err := background.SendTestWebhook(ctx, httpcli.ExternalDoer, args.Description, args.Webhook.URL); err != nil { + logger := r.logger.Scoped("TriggerTestWebhookAction") + + if err := background.SendTestWebhook(ctx, httpcli.ExternalDoer(logger), args.Description, args.Webhook.URL); err != nil { return nil, err } @@ -425,7 +427,8 @@ func (r *Resolver) TriggerTestSlackWebhookAction(ctx context.Context, args *grap return nil, err } - if err := background.SendTestSlackWebhook(ctx, httpcli.ExternalDoer, args.Description, args.SlackWebhook.URL); err != nil { + logger := r.logger.Scoped("TriggerTestSlackWebhookAction") + if err := background.SendTestSlackWebhook(ctx, httpcli.ExternalDoer(logger), args.Description, args.SlackWebhook.URL); err != nil { return nil, err } diff --git a/cmd/frontend/internal/guardrails/init.go b/cmd/frontend/internal/guardrails/init.go index 6e4d4536b6700..69f56c60e6092 100644 --- a/cmd/frontend/internal/guardrails/init.go +++ b/cmd/frontend/internal/guardrails/init.go @@ -23,7 +23,7 @@ import ( // MockHttpClient is used to inject a test double, since the external client used // by default is prevented from hitting localhost, which is useful in testing. -var MockHttpClient httpcli.Doer +var MockHttpClient func(logger log.Logger) httpcli.Doer func Init( _ context.Context, @@ -45,6 +45,7 @@ func Init( initLogic := &enterpriseInitialization{ observationCtx: observationCtx, conf: conf, + logger: observationCtx.Logger.Scoped("enterpriseGuardrails"), } // conf.Watch will first call UpdateService synchronously before // returning. So we can initialize with Unitialized at first. @@ -67,6 +68,8 @@ type enterpriseInitialization struct { client codygateway.Client endpoint string token string + + logger log.Logger } // Service creates an attribution.Service. It tries to get gateway endpoint from site config @@ -75,6 +78,7 @@ type enterpriseInitialization struct { func (e *enterpriseInitialization) Service() attribution.Service { e.mu.Lock() defer e.mu.Unlock() + endpoint, token := confpkg.GetAttributionGateway(e.conf.SiteConfig()) if e.endpoint != endpoint || e.token != token { e.endpoint = endpoint @@ -86,7 +90,9 @@ func (e *enterpriseInitialization) Service() attribution.Service { httpClient = MockHttpClient } - e.client = codygateway.NewClient(httpClient, endpoint, token) + logger := e.logger.Scoped("CodyGatewayClient") + + e.client = codygateway.NewClient(httpClient(logger), endpoint, token) } if e.endpoint == "" || e.token == "" { return attribution.Uninitialized{} diff --git a/cmd/frontend/internal/httpapi/releasecache/cache_test.go b/cmd/frontend/internal/httpapi/releasecache/cache_test.go index 367ca0ea92354..8d1e9dd4c8cb4 100644 --- a/cmd/frontend/internal/httpapi/releasecache/cache_test.go +++ b/cmd/frontend/internal/httpapi/releasecache/cache_test.go @@ -144,5 +144,5 @@ func newTestClient(t *testing.T) *github.V4Client { a := auth.OAuthBearerToken{Token: os.Getenv("GITHUB_TOKEN")} - return github.NewV4Client("https://github.com", u, &a, doer) + return github.NewV4Client("https://github.com", u, &a, doer, logtest.Scoped(t)) } diff --git a/cmd/frontend/internal/httpapi/releasecache/config.go b/cmd/frontend/internal/httpapi/releasecache/config.go index bbf8dc2793511..65fe0863faa2d 100644 --- a/cmd/frontend/internal/httpapi/releasecache/config.go +++ b/cmd/frontend/internal/httpapi/releasecache/config.go @@ -93,7 +93,7 @@ func parseSiteConfig(conf *conf.Unified) (*config, error) { // NewReleaseCache builds a new VersionCache based on the current site config. func (c *config) NewReleaseCache(logger log.Logger) ReleaseCache { - client := github.NewV4Client(c.urn, c.api, &auth.OAuthBearerToken{Token: c.token}, nil) + client := github.NewV4Client(c.urn, c.api, &auth.OAuthBearerToken{Token: c.token}, nil, logger) return newReleaseCache(logger, client, c.owner, c.name) } diff --git a/cmd/frontend/internal/httpapi/src_cli.go b/cmd/frontend/internal/httpapi/src_cli.go index 8408250850092..58944d3079614 100644 --- a/cmd/frontend/internal/httpapi/src_cli.go +++ b/cmd/frontend/internal/httpapi/src_cli.go @@ -57,10 +57,11 @@ type srcCliVersionHandler struct { } func newSrcCliVersionHandler(logger log.Logger) http.Handler { + logger = logger.Scoped("srcCliVersionHandler") return &srcCliVersionHandler{ clock: glock.NewRealClock(), - doer: httpcli.ExternalClient, - logger: logger.Scoped("srcCliVersionHandler"), + doer: httpcli.ExternalClient(logger), + logger: logger, maxStale: srcCliCacheLifetime, } } diff --git a/cmd/gitserver/internal/cloneurl/BUILD.bazel b/cmd/gitserver/internal/cloneurl/BUILD.bazel index 8a382859fefd1..be7decd2b23df 100644 --- a/cmd/gitserver/internal/cloneurl/BUILD.bazel +++ b/cmd/gitserver/internal/cloneurl/BUILD.bazel @@ -28,6 +28,7 @@ go_library( "//internal/types", "//lib/errors", "//schema", + "@com_github_sourcegraph_log//:log", ], ) @@ -50,6 +51,7 @@ go_test( "//internal/timeutil", "//schema", "@com_github_google_go_cmp//cmp", + "@com_github_sourcegraph_log//logtest", "@com_github_stretchr_testify//require", ], ) diff --git a/cmd/gitserver/internal/cloneurl/clone_url.go b/cmd/gitserver/internal/cloneurl/clone_url.go index 2b4f29eafe611..5ac85c3be5d89 100644 --- a/cmd/gitserver/internal/cloneurl/clone_url.go +++ b/cmd/gitserver/internal/cloneurl/clone_url.go @@ -7,6 +7,8 @@ import ( "path" "strings" + "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/internal/conf/reposource" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/encryption/keyring" @@ -29,16 +31,16 @@ import ( "github.com/sourcegraph/sourcegraph/schema" ) -func ForEncryptableConfig(ctx context.Context, db database.DB, kind string, config *extsvc.EncryptableConfig, repo *types.Repo) (string, error) { +func ForEncryptableConfig(ctx context.Context, logger log.Logger, db database.DB, kind string, config *extsvc.EncryptableConfig, repo *types.Repo) (string, error) { parsed, err := extsvc.ParseEncryptableConfig(ctx, kind, config) if err != nil { return "", errors.Wrap(err, "loading service configuration") } - return cloneURL(ctx, db, kind, parsed, repo) + return cloneURL(ctx, logger, db, kind, parsed, repo) } -func cloneURL(ctx context.Context, db database.DB, kind string, parsed any, repo *types.Repo) (string, error) { +func cloneURL(ctx context.Context, logger log.Logger, db database.DB, kind string, parsed any, repo *types.Repo) (string, error) { switch t := parsed.(type) { case *schema.AWSCodeCommitConnection: if r, ok := repo.Metadata.(*awscodecommit.Repository); ok { @@ -62,7 +64,7 @@ func cloneURL(ctx context.Context, db database.DB, kind string, parsed any, repo } case *schema.GitHubConnection: if r, ok := repo.Metadata.(*github.Repository); ok { - return githubCloneURL(ctx, db, r, t) + return githubCloneURL(ctx, logger, db, r, t) } case *schema.GitLabConnection: if r, ok := repo.Metadata.(*gitlab.Project); ok { @@ -199,7 +201,7 @@ func bitbucketCloudCloneURL(repo *bitbucketcloud.Repo, cfg *schema.BitbucketClou return u.String(), nil } -func githubCloneURL(ctx context.Context, db database.DB, repo *github.Repository, cfg *schema.GitHubConnection) (string, error) { +func githubCloneURL(ctx context.Context, logger log.Logger, db database.DB, repo *github.Repository, cfg *schema.GitHubConnection) (string, error) { if cfg.GitURLType == "ssh" { baseURL, err := url.Parse(cfg.Url) if err != nil { @@ -226,8 +228,10 @@ func githubCloneURL(ctx context.Context, db database.DB, repo *github.Repository if err != nil { return "", err } + + logger = logger.Scoped("githubCloneURL.auther") if auther.NeedsRefresh() { - if err := auther.Refresh(ctx, httpcli.ExternalClient); err != nil { + if err := auther.Refresh(ctx, httpcli.ExternalClient(logger)); err != nil { return "", err } } diff --git a/cmd/gitserver/internal/cloneurl/clone_url_test.go b/cmd/gitserver/internal/cloneurl/clone_url_test.go index dfde5c90811c7..ae6530309bbc0 100644 --- a/cmd/gitserver/internal/cloneurl/clone_url_test.go +++ b/cmd/gitserver/internal/cloneurl/clone_url_test.go @@ -8,6 +8,7 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/sourcegraph/log/logtest" "github.com/stretchr/testify/require" "github.com/sourcegraph/sourcegraph/internal/database/dbmocks" @@ -175,7 +176,7 @@ func TestBitbucketCloudCloneURLs(t *testing.T) { func TestGitHubCloneURLs(t *testing.T) { t.Run("empty repo.URL", func(t *testing.T) { - _, err := githubCloneURL(context.Background(), dbmocks.NewMockDB(), &github.Repository{}, &schema.GitHubConnection{}) + _, err := githubCloneURL(context.Background(), logtest.Scoped(t), dbmocks.NewMockDB(), &github.Repository{}, &schema.GitHubConnection{}) got := fmt.Sprintf("%v", err) want := "empty repo.URL" if diff := cmp.Diff(want, got); diff != "" { @@ -208,7 +209,7 @@ func TestGitHubCloneURLs(t *testing.T) { repo.URL = test.RepoURL - got, err := githubCloneURL(context.Background(), dbmocks.NewMockDB(), &repo, &cfg) + got, err := githubCloneURL(context.Background(), logtest.Scoped(t), dbmocks.NewMockDB(), &repo, &cfg) if err != nil { t.Fatal(err) } diff --git a/cmd/gitserver/internal/vcssyncer/syncer.go b/cmd/gitserver/internal/vcssyncer/syncer.go index 37df203d5980c..f273d38d68220 100644 --- a/cmd/gitserver/internal/vcssyncer/syncer.go +++ b/cmd/gitserver/internal/vcssyncer/syncer.go @@ -79,7 +79,8 @@ type NewVCSSyncerOpts struct { GetRemoteURLSource func(ctx context.Context, repo api.RepoName) (RemoteURLSource, error) } -func NewVCSSyncer(ctx context.Context, opts *NewVCSSyncerOpts) (VCSSyncer, error) { +func NewVCSSyncer(ctx context.Context, logger log.Logger, opts *NewVCSSyncerOpts) (VCSSyncer, error) { + logger = logger.Scoped("vcssyncer") // We need an internal actor in case we are trying to access a private repo. We // only need access in order to find out the type of code host we're using, so // it's safe. @@ -131,7 +132,7 @@ func NewVCSSyncer(ctx context.Context, opts *NewVCSSyncerOpts) (VCSSyncer, error if err != nil { return nil, err } - cli, err := npm.NewHTTPClient(urn, c.Registry, c.Credentials, httpcli.ExternalClientFactory) + cli, err := npm.NewHTTPClient(urn, c.Registry, c.Credentials, httpcli.ExternalClientFactory(logger.Scoped("npm"))) if err != nil { return nil, err } @@ -142,7 +143,7 @@ func NewVCSSyncer(ctx context.Context, opts *NewVCSSyncerOpts) (VCSSyncer, error if err != nil { return nil, err } - cli := gomodproxy.NewClient(urn, c.Urls, httpcli.ExternalClientFactory) + cli := gomodproxy.NewClient(urn, c.Urls, httpcli.ExternalClientFactory(logger.Scoped("gomodules"))) return NewGoModulesSyncer(&c, opts.DepsSvc, cli, opts.FS, opts.GetRemoteURLSource), nil case extsvc.TypePythonPackages: var c schema.PythonPackagesConnection @@ -150,7 +151,7 @@ func NewVCSSyncer(ctx context.Context, opts *NewVCSSyncerOpts) (VCSSyncer, error if err != nil { return nil, err } - cli, err := pypi.NewClient(urn, c.Urls, httpcli.ExternalClientFactory) + cli, err := pypi.NewClient(urn, c.Urls, httpcli.ExternalClientFactory(logger.Scoped("python"))) if err != nil { return nil, err } @@ -161,7 +162,7 @@ func NewVCSSyncer(ctx context.Context, opts *NewVCSSyncerOpts) (VCSSyncer, error if err != nil { return nil, err } - cli, err := crates.NewClient(urn, httpcli.ExternalClientFactory) + cli, err := crates.NewClient(urn, httpcli.ExternalClientFactory(logger.Scoped("rust"))) if err != nil { return nil, err } @@ -172,7 +173,7 @@ func NewVCSSyncer(ctx context.Context, opts *NewVCSSyncerOpts) (VCSSyncer, error if err != nil { return nil, err } - cli, err := rubygems.NewClient(urn, c.Repository, httpcli.ExternalClientFactory) + cli, err := rubygems.NewClient(urn, c.Repository, httpcli.ExternalClientFactory(logger.Scoped("ruby"))) if err != nil { return nil, err } diff --git a/cmd/gitserver/internal/vcssyncer/syncer_test.go b/cmd/gitserver/internal/vcssyncer/syncer_test.go index 7635fa71059de..af1c0892ff578 100644 --- a/cmd/gitserver/internal/vcssyncer/syncer_test.go +++ b/cmd/gitserver/internal/vcssyncer/syncer_test.go @@ -57,7 +57,7 @@ func TestGetVCSSyncer(t *testing.T) { }, nil }) - s, err := NewVCSSyncer(context.Background(), &NewVCSSyncerOpts{ + s, err := NewVCSSyncer(context.Background(), logtest.Scoped(t), &NewVCSSyncerOpts{ ExternalServiceStore: extsvcStore, RepoStore: repoStore, DepsSvc: new(dependencies.Service), diff --git a/cmd/gitserver/shared/shared.go b/cmd/gitserver/shared/shared.go index 3f149711ad29c..74c85b5221ee6 100644 --- a/cmd/gitserver/shared/shared.go +++ b/cmd/gitserver/shared/shared.go @@ -60,7 +60,7 @@ type LazyDebugserverEndpoint struct { } func Main(ctx context.Context, observationCtx *observation.Context, ready service.ReadyFunc, debugserverEndpoints *LazyDebugserverEndpoint, config *Config) error { - logger := observationCtx.Logger + logger := observationCtx.Logger.Scoped("gitserver") // Load and validate configuration. if err := config.Validate(); err != nil { @@ -105,7 +105,7 @@ func Main(ctx context.Context, observationCtx *observation.Context, ready servic config.CoursierCacheDir, locker, func(ctx context.Context, repo api.RepoName) (string, error) { - return getRemoteURLFunc(ctx, db, repo) + return getRemoteURLFunc(ctx, logger, db, repo) }, ) @@ -204,12 +204,13 @@ func makeServer( locker internal.RepositoryLocker, getRemoteURLFunc func(ctx context.Context, repo api.RepoName) (string, error), ) *internal.Server { + logger := observationCtx.Logger return server.NewServer(&server.ServerOpts{ - Logger: observationCtx.Logger, + Logger: logger, GitBackendSource: backendSource, GetRemoteURLFunc: getRemoteURLFunc, GetVCSSyncer: func(ctx context.Context, repo api.RepoName) (vcssyncer.VCSSyncer, error) { - return vcssyncer.NewVCSSyncer(ctx, &vcssyncer.NewVCSSyncerOpts{ + return vcssyncer.NewVCSSyncer(ctx, logger, &vcssyncer.NewVCSSyncerOpts{ ExternalServiceStore: db.ExternalServices(), RepoStore: db.Repos(), DepsSvc: dependencies.NewService(observationCtx, db), @@ -307,6 +308,7 @@ func getDB(observationCtx *observation.Context) (*sql.DB, error) { // cloning successfully. func getRemoteURLFunc( ctx context.Context, + logger log.Logger, db database.DB, repo api.RepoName, ) (string, error) { @@ -315,6 +317,10 @@ func getRemoteURLFunc( return "", err } + logger = logger.Scoped("getRemoteURLFunc").With( + log.String("repo", string(repo)), + ) + for _, info := range r.Sources { // build the clone url using the external service config instead of using // the source CloneURL field @@ -323,7 +329,7 @@ func getRemoteURLFunc( return "", err } - return cloneurl.ForEncryptableConfig(ctx, db, svc.Kind, svc.Config, r) + return cloneurl.ForEncryptableConfig(ctx, logger, db, svc.Kind, svc.Config, r) } return "", errors.Errorf("no sources for %q", repo) } diff --git a/cmd/pings/service/service.go b/cmd/pings/service/service.go index 1c43658628315..11d13628c1408 100644 --- a/cmd/pings/service/service.go +++ b/cmd/pings/service/service.go @@ -90,7 +90,7 @@ func (s *serviceState) Healthy(ctx context.Context, query url.Values) error { } if hubspotutil.HasAPIKey() { - if err := hubspotutil.Client().Ping(ctx, 30*time.Second); err != nil { + if err := hubspotutil.Client(s.logger).Ping(ctx, 30*time.Second); err != nil { status["hubspotClient"] = err.Error() s.logger.Error("failed to ping HubSpot client", log.Error(err)) } else { diff --git a/cmd/worker/internal/authz/integration_test.go b/cmd/worker/internal/authz/integration_test.go index 184477ca5c670..1e2e0c7a8f804 100644 --- a/cmd/worker/internal/authz/integration_test.go +++ b/cmd/worker/internal/authz/integration_test.go @@ -524,7 +524,7 @@ func TestIntegration_GitLabPermissions(t *testing.T) { DB: testDB, CLI: doer, SyncInternalRepoPermissions: true, - }) + }, logtest.Scoped(t)) for _, repo := range testRepos { err = reposStore.RepoStore().Create(ctx, &repo) diff --git a/cmd/worker/internal/codygateway/usageworker.go b/cmd/worker/internal/codygateway/usageworker.go index 71ba3037880c1..482ef9d6878d7 100644 --- a/cmd/worker/internal/codygateway/usageworker.go +++ b/cmd/worker/internal/codygateway/usageworker.go @@ -40,7 +40,10 @@ func (j *usageJob) Routines(_ context.Context, observationCtx *observation.Conte func newCodyGatewayUsageRoutine(observationCtx *observation.Context) goroutine.BackgroundRoutine { handler := goroutine.HandlerFunc(func(ctx context.Context) error { - cgc, ok := codygateway.NewClientFromSiteConfig(httpcli.ExternalDoer) + logger := observationCtx.Logger.With( + log.String("routine", "codygateway-usage"), + ) + cgc, ok := codygateway.NewClientFromSiteConfig(httpcli.ExternalDoer(logger)) if !ok { // If no client is configured, skip this iteration. observationCtx.Logger.Info("Not checking Cody Gateway usage, disabled") diff --git a/cmd/worker/internal/licensecheck/check.go b/cmd/worker/internal/licensecheck/check.go index 43d98115a6aaf..b2e6d0c51c2ed 100644 --- a/cmd/worker/internal/licensecheck/check.go +++ b/cmd/worker/internal/licensecheck/check.go @@ -30,12 +30,14 @@ var ( // from dotcom and stores the result in redis. // It re-runs the check if the license key changes. func newLicenseChecker(ctx context.Context, logger log.Logger, db database.DB, kv redispool.KeyValue) goroutine.BackgroundRoutine { + logger = logger.Scoped("licenseChecker") + return goroutine.NewPeriodicGoroutine( ctx, &licenseChecker{ db: db, - doer: httpcli.ExternalDoer, - logger: logger.Scoped("licenseChecker"), + doer: httpcli.ExternalDoer(logger), + logger: logger, kv: kv, }, goroutine.WithName("licensing.check-license-validity"), diff --git a/cmd/worker/internal/outboundwebhooks/job.go b/cmd/worker/internal/outboundwebhooks/job.go index 0a5c3af5f1507..7781b4910063d 100644 --- a/cmd/worker/internal/outboundwebhooks/job.go +++ b/cmd/worker/internal/outboundwebhooks/job.go @@ -50,7 +50,7 @@ func (s *sender) Routines(_ context.Context, observationCtx *observation.Context return []goroutine.BackgroundRoutine{ makeWorker( - ctx, observationCtx, workerStore, client, + ctx, observationCtx, workerStore, client(observationCtx.Logger), database.OutboundWebhooksWith(db, key), database.OutboundWebhookLogsWith(db, key), ), diff --git a/cmd/worker/internal/permissions/bitbucket_projects.go b/cmd/worker/internal/permissions/bitbucket_projects.go index b3d62310f0bac..b8ff81ffc1d77 100644 --- a/cmd/worker/internal/permissions/bitbucket_projects.go +++ b/cmd/worker/internal/permissions/bitbucket_projects.go @@ -157,7 +157,7 @@ func (h *bitbucketProjectPermissionsHandler) getBitbucketClient(ctx context.Cont return nil, err } - return bitbucketserver.NewClient(svc.URN(), &c, cli) + return bitbucketserver.NewClient(svc.URN(), &c, cli, logger) } func (h *bitbucketProjectPermissionsHandler) setReposUnrestricted(ctx context.Context, logger log.Logger, repoIDs []api.RepoID, projectKey string) error { diff --git a/cmd/worker/internal/permissions/bitbucket_projects_test.go b/cmd/worker/internal/permissions/bitbucket_projects_test.go index f00a2b88c15a1..8b82868ff0493 100644 --- a/cmd/worker/internal/permissions/bitbucket_projects_test.go +++ b/cmd/worker/internal/permissions/bitbucket_projects_test.go @@ -349,7 +349,7 @@ func TestHandleRestricted(t *testing.T) { h := bitbucketProjectPermissionsHandler{ db: db, - client: bitbucketserver.NewTestClient(t, "client", false), + client: bitbucketserver.NewTestClient(t, logtest.Scoped(t), "client", false), } // set permissions for 3 users (2 existing, 1 pending) and 2 repos @@ -472,7 +472,7 @@ func TestHandleUnrestricted(t *testing.T) { h := bitbucketProjectPermissionsHandler{ db: db, - client: bitbucketserver.NewTestClient(t, "client", false), + client: bitbucketserver.NewTestClient(t, logtest.Scoped(t), "client", false), } // set permissions for 3 users (2 existing, 1 pending) and 2 repos diff --git a/internal/authz/providers/BUILD.bazel b/internal/authz/providers/BUILD.bazel index 21659cb639899..dd741a062a17e 100644 --- a/internal/authz/providers/BUILD.bazel +++ b/internal/authz/providers/BUILD.bazel @@ -52,6 +52,8 @@ go_test( "@com_github_inconshreveable_log15//:log15", "@com_github_json_iterator_go//:go", "@com_github_kr_pretty//:pretty", + "@com_github_sourcegraph_log//:log", + "@com_github_sourcegraph_log//logtest", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", ], diff --git a/internal/authz/providers/authz.go b/internal/authz/providers/authz.go index b2af784774730..7843025bc0a26 100644 --- a/internal/authz/providers/authz.go +++ b/internal/authz/providers/authz.go @@ -156,12 +156,12 @@ func ProvidersFromConfig( } initResult := github.NewAuthzProviders(ctx, db, gitHubConns, cfg.SiteConfig().AuthProviders, enableGithubInternalRepoVisibility) - initResult.Append(gitlab.NewAuthzProviders(db, gitLabConns, cfg.SiteConfig().AuthProviders)) - initResult.Append(bitbucketserver.NewAuthzProviders(bitbucketServerConns)) + initResult.Append(gitlab.NewAuthzProviders(db, gitLabConns, cfg.SiteConfig().AuthProviders, logger)) + initResult.Append(bitbucketserver.NewAuthzProviders(bitbucketServerConns, logger)) initResult.Append(perforce.NewAuthzProviders(perforceConns)) - initResult.Append(bitbucketcloud.NewAuthzProviders(db, bitbucketCloudConns)) - initResult.Append(gerrit.NewAuthzProviders(gerritConns)) - initResult.Append(azuredevops.NewAuthzProviders(db, azuredevopsConns, httpcli.ExternalClient)) + initResult.Append(bitbucketcloud.NewAuthzProviders(db, bitbucketCloudConns, logger)) + initResult.Append(gerrit.NewAuthzProviders(gerritConns, logger)) + initResult.Append(azuredevops.NewAuthzProviders(db, azuredevopsConns, httpcli.ExternalClient(logger), logger)) return initResult.Providers, initResult.Problems, initResult.Warnings, initResult.InvalidConnections } @@ -171,5 +171,5 @@ var ValidateExternalServiceConfig = database.MakeValidateExternalServiceConfigFu []database.GitLabValidatorFunc{gitlab.ValidateAuthz}, []database.BitbucketServerValidatorFunc{bitbucketserver.ValidateAuthz}, []database.PerforceValidatorFunc{perforce.ValidateAuthz}, - []database.AzureDevOpsValidatorFunc{func(_ *schema.AzureDevOpsConnection) error { return nil }}, + []database.AzureDevOpsValidatorFunc{func(_ *schema.AzureDevOpsConnection, _ log.Logger) error { return nil }}, ) // TODO: @varsanojidan switch this with actual authz once its implemented. diff --git a/internal/authz/providers/authz_test.go b/internal/authz/providers/authz_test.go index 09c490cb762f2..c4c36c09fa106 100644 --- a/internal/authz/providers/authz_test.go +++ b/internal/authz/providers/authz_test.go @@ -12,6 +12,8 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" jsoniter "github.com/json-iterator/go" "github.com/kr/pretty" + "github.com/sourcegraph/log" + "github.com/sourcegraph/log/logtest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -70,10 +72,10 @@ func (m gitlabAuthzProviderParams) FetchRepoPerms(context.Context, *extsvc.Repos func TestAuthzProvidersFromConfig(t *testing.T) { t.Cleanup(licensing.TestingSkipFeatureChecks()) - gitlab.NewOAuthProvider = func(op gitlab.OAuthProviderOp) authz.Provider { + gitlab.NewOAuthProvider = func(op gitlab.OAuthProviderOp, _ log.Logger) authz.Provider { return gitlabAuthzProviderParams{OAuthOp: op} } - gitlab.NewSudoProvider = func(op gitlab.SudoProviderOp) authz.Provider { + gitlab.NewSudoProvider = func(op gitlab.SudoProviderOp, _ log.Logger) authz.Provider { return gitlabAuthzProviderParams{SudoOp: op} } @@ -1911,7 +1913,7 @@ func TestValidateExternalServiceConfig(t *testing.T) { Kind: tc.kind, Config: tc.config, AuthProviders: tc.ps, - }) + }, logtest.Scoped(t)) if err == nil { have = append(have, "") } else { diff --git a/internal/authz/providers/azuredevops/BUILD.bazel b/internal/authz/providers/azuredevops/BUILD.bazel index 4c1154ee88dd0..eb169d14d6953 100644 --- a/internal/authz/providers/azuredevops/BUILD.bazel +++ b/internal/authz/providers/azuredevops/BUILD.bazel @@ -46,5 +46,6 @@ go_test( "//schema", "@com_github_google_go_cmp//cmp", "@com_github_goware_urlx//:urlx", + "@com_github_sourcegraph_log//logtest", ], ) diff --git a/internal/authz/providers/azuredevops/provider.go b/internal/authz/providers/azuredevops/provider.go index 5102b74807466..fdf961336ab9a 100644 --- a/internal/authz/providers/azuredevops/provider.go +++ b/internal/authz/providers/azuredevops/provider.go @@ -24,7 +24,7 @@ import ( var mockServerURL string -func NewAuthzProviders(db database.DB, conns []*types.AzureDevOpsConnection, httpClient *http.Client) *authztypes.ProviderInitResult { +func NewAuthzProviders(db database.DB, conns []*types.AzureDevOpsConnection, httpClient *http.Client, logger log.Logger) *authztypes.ProviderInitResult { orgs, projects := map[string]struct{}{}, map[string]struct{}{} authorizedConnections := []*types.AzureDevOpsConnection{} @@ -58,7 +58,7 @@ func NewAuthzProviders(db database.DB, conns []*types.AzureDevOpsConnection, htt return initResults } - p, err := newAuthzProvider(db, authorizedConnections, orgs, projects, httpClient) + p, err := newAuthzProvider(db, authorizedConnections, orgs, projects, httpClient, logger) if err != nil { initResults.InvalidConnections = append(initResults.InvalidConnections, extsvc.TypeAzureDevOps) initResults.Problems = append(initResults.Problems, err.Error()) @@ -69,7 +69,7 @@ func NewAuthzProviders(db database.DB, conns []*types.AzureDevOpsConnection, htt return initResults } -func newAuthzProvider(db database.DB, conns []*types.AzureDevOpsConnection, orgs, projects map[string]struct{}, httpClient *http.Client) (*Provider, error) { +func newAuthzProvider(db database.DB, conns []*types.AzureDevOpsConnection, orgs, projects map[string]struct{}, httpClient *http.Client, logger log.Logger) (*Provider, error) { if err := licensing.Check(licensing.FeatureACLs); err != nil { return nil, err } @@ -87,6 +87,7 @@ func newAuthzProvider(db database.DB, conns []*types.AzureDevOpsConnection, orgs orgs: orgs, projects: projects, httpClient: httpClient, + logger: logger.Scoped("AzureDevOpsAuthzProvider"), }, nil } @@ -103,6 +104,7 @@ type Provider struct { // projects is the set of projects as configured across all the code host connections. projects map[string]struct{} httpClient *http.Client + logger log.Logger } func (p *Provider) FetchAccount(_ context.Context, _ *types.User, _ []*extsvc.Account, _ []string) (*extsvc.Account, error) { @@ -148,6 +150,7 @@ func (p *Provider) FetchUserPerms(ctx context.Context, account *extsvc.Account, apiURL, oauthToken, p.httpClient, + logger, ) if err != nil { return nil, errors.Wrapf( @@ -295,6 +298,7 @@ func (p *Provider) ValidateConnection(ctx context.Context) error { Password: conn.Token, }, p.httpClient, + p.logger, ) if err != nil { allErrors = append(allErrors, fmt.Sprintf("%s:%s", conn.URN, err.Error())) diff --git a/internal/authz/providers/azuredevops/provider_test.go b/internal/authz/providers/azuredevops/provider_test.go index 5ff5f1447b25e..e9213ad8aca10 100644 --- a/internal/authz/providers/azuredevops/provider_test.go +++ b/internal/authz/providers/azuredevops/provider_test.go @@ -10,6 +10,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/goware/urlx" + "github.com/sourcegraph/log/logtest" "github.com/sourcegraph/sourcegraph/internal/authz" "github.com/sourcegraph/sourcegraph/internal/conf" @@ -118,7 +119,7 @@ func TestProvider_NewAuthzProviders(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { licensing.MockCheckFeature = tc.mockCheckFeature - result := NewAuthzProviders(db, tc.connections, httpcli.TestExternalClient) + result := NewAuthzProviders(db, tc.connections, httpcli.TestExternalClient, logtest.Scoped(t)) if diff := cmp.Diff(tc.expectedInvalidConnections, result.InvalidConnections); diff != "" { t.Errorf("mismatched InvalidConnections (-want, +got)\n%s", diff) @@ -643,7 +644,7 @@ func TestProvider_FetchUserPerms(t *testing.T) { URN: "", AzureDevOpsConnection: tc.connection, }, - }, httpcli.TestExternalClient) + }, httpcli.TestExternalClient, logtest.Scoped(t)) // We don't need to test for the inner type yet. Asserting the length is sufficient. if len(expectedProviders) != len(result.Providers) { @@ -729,7 +730,7 @@ func TestProvider_FetchUserPerms(t *testing.T) { Projects: []string{"solar/system", "milky/way"}, }, }, - }, httpcli.TestExternalClient) + }, httpcli.TestExternalClient, logtest.Scoped(t)) if len(result.Providers) == 0 { t.Fatal("No providers found, expected one") @@ -893,7 +894,7 @@ func Test_ValidateConnection(t *testing.T) { Projects: []string{"solar/system", "milky/way"}, }, }, - }, httpcli.TestExternalClient) + }, httpcli.TestExternalClient, logtest.Scoped(t)) if len(result.Providers) == 0 { fmt.Println(result) diff --git a/internal/authz/providers/bitbucketcloud/BUILD.bazel b/internal/authz/providers/bitbucketcloud/BUILD.bazel index b9184694cdc3e..9bb0648b7a37f 100644 --- a/internal/authz/providers/bitbucketcloud/BUILD.bazel +++ b/internal/authz/providers/bitbucketcloud/BUILD.bazel @@ -23,6 +23,7 @@ go_library( "//internal/types", "//lib/errors", "//schema", + "@com_github_sourcegraph_log//:log", ], ) @@ -48,6 +49,7 @@ go_test( "//internal/types", "//schema", "@com_github_google_go_cmp//cmp", + "@com_github_sourcegraph_log//logtest", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", "@org_golang_x_oauth2//:oauth2", diff --git a/internal/authz/providers/bitbucketcloud/authz.go b/internal/authz/providers/bitbucketcloud/authz.go index b9e8e6214eca6..5a0326b27ce7d 100644 --- a/internal/authz/providers/bitbucketcloud/authz.go +++ b/internal/authz/providers/bitbucketcloud/authz.go @@ -1,6 +1,8 @@ package bitbucketcloud import ( + "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/internal/licensing" "github.com/sourcegraph/sourcegraph/internal/authz" @@ -21,11 +23,11 @@ import ( // This constructor does not and should not directly check connectivity to external services - if // desired, callers should use `(*Provider).ValidateConnection` directly to get warnings related // to connection issues. -func NewAuthzProviders(db database.DB, conns []*types.BitbucketCloudConnection) *atypes.ProviderInitResult { +func NewAuthzProviders(db database.DB, conns []*types.BitbucketCloudConnection, logger log.Logger) *atypes.ProviderInitResult { initResults := &atypes.ProviderInitResult{} for _, c := range conns { - p, err := newAuthzProvider(db, c) + p, err := newAuthzProvider(db, c, logger) if err != nil { initResults.InvalidConnections = append(initResults.InvalidConnections, extsvc.TypeBitbucketCloud) initResults.Problems = append(initResults.Problems, err.Error()) @@ -43,6 +45,7 @@ func NewAuthzProviders(db database.DB, conns []*types.BitbucketCloudConnection) func newAuthzProvider( db database.DB, c *types.BitbucketCloudConnection, + logger log.Logger, ) (authz.Provider, error) { // If authorization is not set for this connection, we do not need an // authz provider. @@ -53,14 +56,14 @@ func newAuthzProvider( return nil, err } - bbClient, err := bitbucketcloud.NewClient(c.URN, c.BitbucketCloudConnection, nil) + bbClient, err := bitbucketcloud.NewClient(c.URN, c.BitbucketCloudConnection, nil, logger) if err != nil { return nil, err } return NewProvider(db, c, ProviderOptions{ BitbucketCloudClient: bbClient, - }), nil + }, logger), nil } // ValidateAuthz validates the authorization fields of the given Perforce diff --git a/internal/authz/providers/bitbucketcloud/authz_test.go b/internal/authz/providers/bitbucketcloud/authz_test.go index b826a898a78ff..033c7f2d7d10f 100644 --- a/internal/authz/providers/bitbucketcloud/authz_test.go +++ b/internal/authz/providers/bitbucketcloud/authz_test.go @@ -3,6 +3,7 @@ package bitbucketcloud import ( "testing" + "github.com/sourcegraph/log/logtest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -23,6 +24,7 @@ func TestNewAuthzProviders(t *testing.T) { Url: schema.DefaultBitbucketCloudURL, }, }}, + logtest.Scoped(t), ) assertion := assert.New(t) @@ -45,6 +47,7 @@ func TestNewAuthzProviders(t *testing.T) { }, }, }, + logtest.Scoped(t), ) require.Len(t, initResults.Providers, 1, "expect exactly one provider") @@ -67,6 +70,7 @@ func TestNewAuthzProviders(t *testing.T) { }, }, }, + logtest.Scoped(t), ) expectedError := []string{"failed"} diff --git a/internal/authz/providers/bitbucketcloud/provider.go b/internal/authz/providers/bitbucketcloud/provider.go index 9e4042b3a38fb..43ee6a6b00288 100644 --- a/internal/authz/providers/bitbucketcloud/provider.go +++ b/internal/authz/providers/bitbucketcloud/provider.go @@ -6,6 +6,8 @@ import ( "net/url" "strings" + "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/internal/authz" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/extsvc" @@ -25,6 +27,7 @@ type Provider struct { client bitbucketcloud.Client pageSize int // Page size to use in paginated requests. db database.DB + logger log.Logger } type ProviderOptions struct { @@ -37,14 +40,15 @@ var _ authz.Provider = (*Provider)(nil) // the given bitbucket.Client to talk to the Bitbucket Cloud API that is // the source of truth for permissions. Sourcegraph users will need a valid // Bitbucket Cloud external account for permissions to sync correctly. -func NewProvider(db database.DB, conn *types.BitbucketCloudConnection, opts ProviderOptions) *Provider { +func NewProvider(db database.DB, conn *types.BitbucketCloudConnection, opts ProviderOptions, logger log.Logger) *Provider { baseURL, err := url.Parse(conn.Url) if err != nil { return nil } + logger = logger.Scoped("BitbucketCloudAuthzProvider") if opts.BitbucketCloudClient == nil { - opts.BitbucketCloudClient, err = bitbucketcloud.NewClient(conn.URN, conn.BitbucketCloudConnection, httpcli.ExternalClient) + opts.BitbucketCloudClient, err = bitbucketcloud.NewClient(conn.URN, conn.BitbucketCloudConnection, httpcli.ExternalClient(logger), logger) if err != nil { return nil } @@ -56,6 +60,7 @@ func NewProvider(db database.DB, conn *types.BitbucketCloudConnection, opts Prov client: opts.BitbucketCloudClient, pageSize: 1000, db: db, + logger: logger, } } diff --git a/internal/authz/providers/bitbucketcloud/provider_test.go b/internal/authz/providers/bitbucketcloud/provider_test.go index c92449d02ae47..8769a84d83356 100644 --- a/internal/authz/providers/bitbucketcloud/provider_test.go +++ b/internal/authz/providers/bitbucketcloud/provider_test.go @@ -11,6 +11,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/sourcegraph/log/logtest" "golang.org/x/oauth2" "github.com/sourcegraph/sourcegraph/internal/authz" @@ -78,7 +79,7 @@ func TestProvider_FetchUserPerms(t *testing.T) { ApiURL: "https://bitbucket.org", Url: "https://bitbucket.org", }, - }, ProviderOptions{}) + }, ProviderOptions{}, logtest.Scoped(t)) _, err := p.FetchUserPerms(context.Background(), nil, authz.FetchPermsOptions{}) want := "no account provided" got := fmt.Sprintf("%v", err) @@ -94,7 +95,7 @@ func TestProvider_FetchUserPerms(t *testing.T) { ApiURL: "https://bitbucket.org", Url: "https://bitbucket.org", }, - }, ProviderOptions{}) + }, ProviderOptions{}, logtest.Scoped(t)) _, err := p.FetchUserPerms(context.Background(), &extsvc.Account{ AccountSpec: extsvc.AccountSpec{ @@ -118,7 +119,7 @@ func TestProvider_FetchUserPerms(t *testing.T) { ApiURL: "https://bitbucket.org", Url: "https://bitbucket.org", }, - }, ProviderOptions{}) + }, ProviderOptions{}, logtest.Scoped(t)) _, err := p.FetchUserPerms(context.Background(), &extsvc.Account{ AccountSpec: extsvc.AccountSpec{ @@ -143,7 +144,7 @@ func TestProvider_FetchUserPerms(t *testing.T) { ApiURL: server.URL, Url: server.URL, } - client, err := bitbucketcloud.NewClient(server.URL, conn, http.DefaultClient) + client, err := bitbucketcloud.NewClient(server.URL, conn, http.DefaultClient, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -151,7 +152,7 @@ func TestProvider_FetchUserPerms(t *testing.T) { p := NewProvider(db, &types.BitbucketCloudConnection{ BitbucketCloudConnection: conn, - }, ProviderOptions{BitbucketCloudClient: client}) + }, ProviderOptions{BitbucketCloudClient: client}, logtest.Scoped(t)) var acctData extsvc.AccountData err = bitbucketcloud.SetExternalAccountData(&acctData, &bitbucketcloud.Account{}, &oauth2.Token{AccessToken: "my-access-token"}) @@ -189,7 +190,7 @@ func TestProvider_FetchRepoPerms(t *testing.T) { ApiURL: server.URL, Url: server.URL, } - client, err := bitbucketcloud.NewClient(server.URL, conn, http.DefaultClient) + client, err := bitbucketcloud.NewClient(server.URL, conn, http.DefaultClient, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -197,7 +198,7 @@ func TestProvider_FetchRepoPerms(t *testing.T) { p := NewProvider(db, &types.BitbucketCloudConnection{ BitbucketCloudConnection: conn, - }, ProviderOptions{BitbucketCloudClient: client}) + }, ProviderOptions{BitbucketCloudClient: client}, logtest.Scoped(t)) perms, err := p.FetchRepoPerms(context.Background(), &extsvc.Repository{ URI: "bitbucket.org/user/repo", diff --git a/internal/authz/providers/bitbucketserver/BUILD.bazel b/internal/authz/providers/bitbucketserver/BUILD.bazel index 7168649b68d79..e8b4684621593 100644 --- a/internal/authz/providers/bitbucketserver/BUILD.bazel +++ b/internal/authz/providers/bitbucketserver/BUILD.bazel @@ -21,6 +21,7 @@ go_library( "//internal/types", "//lib/errors", "//schema", + "@com_github_sourcegraph_log//:log", "@io_opentelemetry_go_otel//attribute", ], ) @@ -45,5 +46,6 @@ go_test( "//internal/types", "@com_github_google_go_cmp//cmp", "@com_github_inconshreveable_log15//:log15", + "@com_github_sourcegraph_log//logtest", ], ) diff --git a/internal/authz/providers/bitbucketserver/authz.go b/internal/authz/providers/bitbucketserver/authz.go index dfafb7e88593a..ac7f6b6452f22 100644 --- a/internal/authz/providers/bitbucketserver/authz.go +++ b/internal/authz/providers/bitbucketserver/authz.go @@ -1,6 +1,8 @@ package bitbucketserver import ( + "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/internal/authz" atypes "github.com/sourcegraph/sourcegraph/internal/authz/types" "github.com/sourcegraph/sourcegraph/internal/extsvc" @@ -22,12 +24,13 @@ import ( // to connection issues. func NewAuthzProviders( conns []*types.BitbucketServerConnection, + logger log.Logger, ) *atypes.ProviderInitResult { initResults := &atypes.ProviderInitResult{} // Authorization (i.e., permissions) providers for _, c := range conns { pluginPerm := c.Plugin != nil && c.Plugin.Permissions == "enabled" - p, err := newAuthzProvider(c, pluginPerm) + p, err := newAuthzProvider(c, pluginPerm, logger) if err != nil { initResults.InvalidConnections = append(initResults.InvalidConnections, extsvc.TypeBitbucketServer) initResults.Problems = append(initResults.Problems, err.Error()) @@ -42,6 +45,7 @@ func NewAuthzProviders( func newAuthzProvider( c *types.BitbucketServerConnection, pluginPerm bool, + logger log.Logger, ) (authz.Provider, error) { if c.Authorization == nil { return nil, nil @@ -53,7 +57,8 @@ func newAuthzProvider( var errs error - cli, err := bitbucketserver.NewClient(c.URN, c.BitbucketServerConnection, nil) + logger = logger.Scoped("BitbucketServerAuthzProvider") + cli, err := bitbucketserver.NewClient(c.URN, c.BitbucketServerConnection, nil, logger) if err != nil { errs = errors.Append(errs, err) return nil, errs @@ -72,7 +77,7 @@ func newAuthzProvider( // ValidateAuthz validates the authorization fields of the given BitbucketServer external // service config. -func ValidateAuthz(c *schema.BitbucketServerConnection) error { - _, err := newAuthzProvider(&types.BitbucketServerConnection{BitbucketServerConnection: c}, false) +func ValidateAuthz(c *schema.BitbucketServerConnection, logger log.Logger) error { + _, err := newAuthzProvider(&types.BitbucketServerConnection{BitbucketServerConnection: c}, false, logger) return err } diff --git a/internal/authz/providers/bitbucketserver/provider_test.go b/internal/authz/providers/bitbucketserver/provider_test.go index 09287dc7b0665..36f1a47afae04 100644 --- a/internal/authz/providers/bitbucketserver/provider_test.go +++ b/internal/authz/providers/bitbucketserver/provider_test.go @@ -13,6 +13,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/sourcegraph/log/logtest" "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/authz" @@ -489,7 +490,7 @@ func (h codeHost) externalAccount(userID int32, u *bitbucketserver.User) *extsvc } func newClient(t *testing.T, name string) *bitbucketserver.Client { - cli := bitbucketserver.NewTestClient(t, name, *update) + cli := bitbucketserver.NewTestClient(t, logtest.Scoped(t), name, *update) signingKey := os.Getenv("BITBUCKET_SERVER_SIGNING_KEY") if signingKey == "" { diff --git a/internal/authz/providers/gerrit/BUILD.bazel b/internal/authz/providers/gerrit/BUILD.bazel index cfff984a8309f..e96608a07b0b0 100644 --- a/internal/authz/providers/gerrit/BUILD.bazel +++ b/internal/authz/providers/gerrit/BUILD.bazel @@ -20,6 +20,7 @@ go_library( "//internal/licensing", "//internal/types", "//lib/errors", + "@com_github_sourcegraph_log//:log", ], ) diff --git a/internal/authz/providers/gerrit/authz.go b/internal/authz/providers/gerrit/authz.go index e855bd1570fde..4ba6a9e04279e 100644 --- a/internal/authz/providers/gerrit/authz.go +++ b/internal/authz/providers/gerrit/authz.go @@ -1,6 +1,8 @@ package gerrit import ( + "github.com/sourcegraph/log" + atypes "github.com/sourcegraph/sourcegraph/internal/authz/types" "github.com/sourcegraph/sourcegraph/internal/extsvc" "github.com/sourcegraph/sourcegraph/internal/licensing" @@ -8,9 +10,11 @@ import ( ) // NewAuthzProviders returns the set of Gerrit authz providers derived from the connections. -func NewAuthzProviders(conns []*types.GerritConnection) *atypes.ProviderInitResult { +func NewAuthzProviders(conns []*types.GerritConnection, logger log.Logger) *atypes.ProviderInitResult { initResults := &atypes.ProviderInitResult{} + logger = logger.Scoped("GerritAuthzProvider") + for _, c := range conns { if c.Authorization == nil { // No authorization required @@ -23,7 +27,7 @@ func NewAuthzProviders(conns []*types.GerritConnection) *atypes.ProviderInitResu continue } - p, err := NewProvider(c) + p, err := NewProvider(c, logger) if err != nil { initResults.InvalidConnections = append(initResults.InvalidConnections, extsvc.TypeGerrit) initResults.Problems = append(initResults.Problems, err.Error()) diff --git a/internal/authz/providers/gerrit/gerrit.go b/internal/authz/providers/gerrit/gerrit.go index ffa66c5121a80..3f3042639691b 100644 --- a/internal/authz/providers/gerrit/gerrit.go +++ b/internal/authz/providers/gerrit/gerrit.go @@ -4,6 +4,8 @@ import ( "context" "net/url" + "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/internal/authz" "github.com/sourcegraph/sourcegraph/internal/extsvc" "github.com/sourcegraph/sourcegraph/internal/extsvc/auth" @@ -22,15 +24,19 @@ type Provider struct { codeHost *extsvc.CodeHost } -func NewProvider(conn *types.GerritConnection) (*Provider, error) { +func NewProvider(conn *types.GerritConnection, logger log.Logger) (*Provider, error) { baseURL, err := url.Parse(conn.Url) if err != nil { return nil, err } + + logger = logger.Scoped("GerritAuthzProvider"). + With(log.String("url", baseURL.String())) + gClient, err := gerrit.NewClient(conn.URN, baseURL, &gerrit.AccountCredentials{ Username: conn.Username, Password: conn.Password, - }, nil) + }, nil, logger) if err != nil { return nil, err } diff --git a/internal/authz/providers/gitlab/BUILD.bazel b/internal/authz/providers/gitlab/BUILD.bazel index cdaee980f23db..f8ac286685497 100644 --- a/internal/authz/providers/gitlab/BUILD.bazel +++ b/internal/authz/providers/gitlab/BUILD.bazel @@ -25,6 +25,7 @@ go_library( "//internal/types", "//lib/errors", "//schema", + "@com_github_sourcegraph_log//:log", ], ) @@ -59,6 +60,7 @@ go_test( "@com_github_google_go_cmp//cmp", "@com_github_inconshreveable_log15//:log15", "@com_github_sergi_go_diff//diffmatchpatch", + "@com_github_sourcegraph_log//logtest", "@com_github_stretchr_testify//require", "@org_golang_x_oauth2//:oauth2", ], diff --git a/internal/authz/providers/gitlab/authz.go b/internal/authz/providers/gitlab/authz.go index e646a2b9a4eaf..3304a20b4475e 100644 --- a/internal/authz/providers/gitlab/authz.go +++ b/internal/authz/providers/gitlab/authz.go @@ -3,6 +3,8 @@ package gitlab import ( "net/url" + "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/internal/authz" atypes "github.com/sourcegraph/sourcegraph/internal/authz/types" "github.com/sourcegraph/sourcegraph/internal/database" @@ -27,11 +29,12 @@ func NewAuthzProviders( db database.DB, conns []*types.GitLabConnection, ap []schema.AuthProviders, + logger log.Logger, ) *atypes.ProviderInitResult { initResults := &atypes.ProviderInitResult{} // Authorization (i.e., permissions) providers for _, c := range conns { - p, err := newAuthzProvider(db, c, ap) + p, err := newAuthzProvider(db, c, ap, logger) if err != nil { initResults.InvalidConnections = append(initResults.InvalidConnections, extsvc.TypeGitLab) initResults.Problems = append(initResults.Problems, err.Error()) @@ -43,7 +46,7 @@ func NewAuthzProviders( return initResults } -func newAuthzProvider(db database.DB, c *types.GitLabConnection, ps []schema.AuthProviders) (authz.Provider, error) { +func newAuthzProvider(db database.DB, c *types.GitLabConnection, ps []schema.AuthProviders, logger log.Logger) (authz.Provider, error) { if c.Authorization == nil { return nil, nil } @@ -93,32 +96,32 @@ func newAuthzProvider(db database.DB, c *types.GitLabConnection, ps []schema.Aut TokenType: gitlab.TokenType(c.TokenType), DB: db, SyncInternalRepoPermissions: syncInternalRepoPermissions, - }), nil + }, logger), nil case idp.Username != nil: return NewSudoProvider(SudoProviderOp{ URN: c.URN, BaseURL: glURL, SudoToken: c.Token, SyncInternalRepoPermissions: !c.MarkInternalReposAsPublic, - }), nil + }, logger), nil default: return nil, errors.Errorf("No identityProvider was specified") } } // NewOAuthProvider is a mockable constructor for new OAuthProvider instances. -var NewOAuthProvider = func(op OAuthProviderOp) authz.Provider { - return newOAuthProvider(op, op.CLI) +var NewOAuthProvider = func(op OAuthProviderOp, logger log.Logger) authz.Provider { + return newOAuthProvider(op, op.CLI, logger) } // NewSudoProvider is a mockable constructor for new SudoProvider instances. -var NewSudoProvider = func(op SudoProviderOp) authz.Provider { - return newSudoProvider(op, nil) +var NewSudoProvider = func(op SudoProviderOp, logger log.Logger) authz.Provider { + return newSudoProvider(op, nil, logger) } // ValidateAuthz validates the authorization fields of the given GitLab external // service config. -func ValidateAuthz(cfg *schema.GitLabConnection, ps []schema.AuthProviders) error { - _, err := newAuthzProvider(nil, &types.GitLabConnection{GitLabConnection: cfg}, ps) +func ValidateAuthz(cfg *schema.GitLabConnection, ps []schema.AuthProviders, logger log.Logger) error { + _, err := newAuthzProvider(nil, &types.GitLabConnection{GitLabConnection: cfg}, ps, logger) return err } diff --git a/internal/authz/providers/gitlab/oauth.go b/internal/authz/providers/gitlab/oauth.go index 829ecc5989113..6532e2d807f14 100644 --- a/internal/authz/providers/gitlab/oauth.go +++ b/internal/authz/providers/gitlab/oauth.go @@ -5,6 +5,8 @@ import ( "net/url" "strings" + "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/internal/authz" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/extsvc" @@ -31,6 +33,8 @@ type OAuthProvider struct { db database.DB syncInternalRepoPermissions bool + + logger log.Logger } type OAuthProviderOp struct { @@ -55,17 +59,19 @@ type OAuthProviderOp struct { SyncInternalRepoPermissions bool } -func newOAuthProvider(op OAuthProviderOp, cli httpcli.Doer) *OAuthProvider { +func newOAuthProvider(op OAuthProviderOp, cli httpcli.Doer, logger log.Logger) *OAuthProvider { + logger = logger.Scoped("GitLabAuthzProvider") return &OAuthProvider{ token: op.Token, tokenType: op.TokenType, urn: op.URN, - clientProvider: gitlab.NewClientProvider(op.URN, op.BaseURL, cli), + clientProvider: gitlab.NewClientProvider(op.URN, op.BaseURL, cli, logger), clientURL: op.BaseURL, codeHost: extsvc.NewCodeHost(op.BaseURL, extsvc.TypeGitLab), db: op.DB, syncInternalRepoPermissions: op.SyncInternalRepoPermissions, + logger: logger, } } diff --git a/internal/authz/providers/gitlab/oauth_test.go b/internal/authz/providers/gitlab/oauth_test.go index 15cb66895ee03..da3605b85807b 100644 --- a/internal/authz/providers/gitlab/oauth_test.go +++ b/internal/authz/providers/gitlab/oauth_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/sourcegraph/log/logtest" "github.com/stretchr/testify/require" "golang.org/x/oauth2" @@ -39,7 +40,7 @@ func TestOAuthProvider_FetchUserPerms(t *testing.T) { t.Run("nil account", func(t *testing.T) { p := newOAuthProvider(OAuthProviderOp{ BaseURL: mustURL(t, "https://gitlab.com"), - }, nil) + }, nil, logtest.Scoped(t)) _, err := p.FetchUserPerms(context.Background(), nil, authz.FetchPermsOptions{}) want := "no account provided" got := fmt.Sprintf("%v", err) @@ -51,7 +52,7 @@ func TestOAuthProvider_FetchUserPerms(t *testing.T) { t.Run("not the code host of the account", func(t *testing.T) { p := newOAuthProvider(OAuthProviderOp{ BaseURL: mustURL(t, "https://gitlab.com"), - }, nil) + }, nil, logtest.Scoped(t)) _, err := p.FetchUserPerms(context.Background(), &extsvc.Account{ AccountSpec: extsvc.AccountSpec{ @@ -109,6 +110,7 @@ func TestOAuthProvider_FetchUserPerms(t *testing.T) { }, nil }, }, + logtest.Scoped(t), ) gitlab.MockGetOAuthContext = func() *oauthutil.OAuthContext { @@ -190,6 +192,7 @@ func TestOAuthProvider_FetchUserPerms(t *testing.T) { }, nil }, }, + logtest.Scoped(t), ) gitlab.MockGetOAuthContext = func() *oauthutil.OAuthContext { @@ -254,6 +257,7 @@ func TestOAuthProvider_FetchRepoPerms(t *testing.T) { TokenType: gitlab.TokenTypePAT, }, nil, + logtest.Scoped(t), ) _, err := p.FetchRepoPerms(context.Background(), diff --git a/internal/authz/providers/gitlab/sudo.go b/internal/authz/providers/gitlab/sudo.go index 99ad38a820e04..4d2c1ccbe6ca8 100644 --- a/internal/authz/providers/gitlab/sudo.go +++ b/internal/authz/providers/gitlab/sudo.go @@ -7,6 +7,8 @@ import ( "strconv" "time" + "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/internal/authz" "github.com/sourcegraph/sourcegraph/internal/extsvc" "github.com/sourcegraph/sourcegraph/internal/extsvc/gitlab" @@ -49,12 +51,14 @@ type SudoProviderOp struct { SyncInternalRepoPermissions bool } -func newSudoProvider(op SudoProviderOp, cli httpcli.Doer) *SudoProvider { +func newSudoProvider(op SudoProviderOp, cli httpcli.Doer, logger log.Logger) *SudoProvider { + logger = logger.Scoped("GitLabSudoAuthzProvider") + return &SudoProvider{ sudoToken: op.SudoToken, urn: op.URN, - clientProvider: gitlab.NewClientProvider(op.URN, op.BaseURL, cli), + clientProvider: gitlab.NewClientProvider(op.URN, op.BaseURL, cli, logger), clientURL: op.BaseURL, codeHost: extsvc.NewCodeHost(op.BaseURL, extsvc.TypeGitLab), syncInternalRepoPermissions: op.SyncInternalRepoPermissions, diff --git a/internal/authz/providers/gitlab/sudo_test.go b/internal/authz/providers/gitlab/sudo_test.go index 591d59250216c..9121a7faf61b2 100644 --- a/internal/authz/providers/gitlab/sudo_test.go +++ b/internal/authz/providers/gitlab/sudo_test.go @@ -13,6 +13,7 @@ import ( "github.com/davecgh/go-spew/spew" "github.com/google/go-cmp/cmp" "github.com/sergi/go-diff/diffmatchpatch" + "github.com/sourcegraph/log/logtest" "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/authz" @@ -95,7 +96,7 @@ func Test_GitLab_FetchAccount(t *testing.T) { test := test t.Run(test.description, func(t *testing.T) { ctx := context.Background() - authzProvider := newSudoProvider(test.op, nil) + authzProvider := newSudoProvider(test.op, nil, logtest.Scoped(t)) for _, c := range test.calls { t.Run(c.description, func(t *testing.T) { acct, err := authzProvider.FetchAccount(ctx, c.user, c.current, nil) @@ -122,7 +123,7 @@ func TestSudoProvider_FetchUserPerms(t *testing.T) { t.Run("nil account", func(t *testing.T) { p := newSudoProvider(SudoProviderOp{ BaseURL: mustURL(t, "https://gitlab.com"), - }, nil) + }, nil, logtest.Scoped(t)) _, err := p.FetchUserPerms(context.Background(), nil, authz.FetchPermsOptions{}) want := "no account provided" got := fmt.Sprintf("%v", err) @@ -134,7 +135,7 @@ func TestSudoProvider_FetchUserPerms(t *testing.T) { t.Run("not the code host of the account", func(t *testing.T) { p := newSudoProvider(SudoProviderOp{ BaseURL: mustURL(t, "https://gitlab.com"), - }, nil) + }, nil, logtest.Scoped(t)) _, err := p.FetchUserPerms(context.Background(), &extsvc.Account{ AccountSpec: extsvc.AccountSpec{ @@ -197,6 +198,7 @@ func TestSudoProvider_FetchUserPerms(t *testing.T) { }, nil }, }, + logtest.Scoped(t), ) accountData := json.RawMessage(`{"id": 999}`) @@ -270,6 +272,7 @@ func TestSudoProvider_FetchUserPerms(t *testing.T) { }, nil }, }, + logtest.Scoped(t), ) accountData := json.RawMessage(`{"id": 999}`) @@ -317,7 +320,7 @@ func TestSudoProvider_FetchRepoPerms(t *testing.T) { t.Run("nil repository", func(t *testing.T) { p := newSudoProvider(SudoProviderOp{ BaseURL: mustURL(t, "https://gitlab.com"), - }, nil) + }, nil, logtest.Scoped(t)) _, err := p.FetchRepoPerms(context.Background(), nil, authz.FetchPermsOptions{}) want := "no repository provided" got := fmt.Sprintf("%v", err) @@ -329,7 +332,7 @@ func TestSudoProvider_FetchRepoPerms(t *testing.T) { t.Run("not the code host of the repository", func(t *testing.T) { p := newSudoProvider(SudoProviderOp{ BaseURL: mustURL(t, "https://gitlab.com"), - }, nil) + }, nil, logtest.Scoped(t)) _, err := p.FetchRepoPerms(context.Background(), &extsvc.Repository{ URI: "https://github.com/user/repo", @@ -383,6 +386,7 @@ func TestSudoProvider_FetchRepoPerms(t *testing.T) { }, nil }, }, + logtest.Scoped(t), ) accountIDs, err := p.FetchRepoPerms(context.Background(), diff --git a/internal/batches/sources/BUILD.bazel b/internal/batches/sources/BUILD.bazel index df063b4e34f0f..299c8c25ad86a 100644 --- a/internal/batches/sources/BUILD.bazel +++ b/internal/batches/sources/BUILD.bazel @@ -124,6 +124,8 @@ go_test( "@com_github_grafana_regexp//:regexp", "@com_github_inconshreveable_log15//:log15", "@com_github_masterminds_semver//:semver", + "@com_github_sourcegraph_log//:log", + "@com_github_sourcegraph_log//logtest", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//mock", "@com_github_stretchr_testify//require", diff --git a/internal/batches/sources/azuredevops.go b/internal/batches/sources/azuredevops.go index 1bd6a33004197..30ce28c1cfac5 100644 --- a/internal/batches/sources/azuredevops.go +++ b/internal/batches/sources/azuredevops.go @@ -6,6 +6,8 @@ import ( "strconv" "strings" + logger "github.com/sourcegraph/log" + btypes "github.com/sourcegraph/sourcegraph/internal/batches/types" "github.com/sourcegraph/sourcegraph/internal/conf" "github.com/sourcegraph/sourcegraph/internal/gitserver/gitdomain" @@ -24,11 +26,14 @@ import ( type AzureDevOpsSource struct { client azuredevops.Client + logger logger.Logger } -var _ ForkableChangesetSource = AzureDevOpsSource{} +var _ ForkableChangesetSource = &AzureDevOpsSource{} + +func NewAzureDevOpsSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory, logger logger.Logger) (*AzureDevOpsSource, error) { + logger = logger.Scoped("AzureDevOpsSource") -func NewAzureDevOpsSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory) (*AzureDevOpsSource, error) { rawConfig, err := svc.Config.Decrypt(ctx) if err != nil { return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err) @@ -39,7 +44,7 @@ func NewAzureDevOpsSource(ctx context.Context, svc *types.ExternalService, cf *h } if cf == nil { - cf = httpcli.ExternalClientFactory + cf = httpcli.ExternalClientFactory(logger) } cli, err := cf.Doer() @@ -47,28 +52,28 @@ func NewAzureDevOpsSource(ctx context.Context, svc *types.ExternalService, cf *h return nil, errors.Wrap(err, "creating external client") } - client, err := azuredevops.NewClient(svc.URN(), c.Url, &auth.BasicAuth{Username: c.Username, Password: c.Token}, cli) + client, err := azuredevops.NewClient(svc.URN(), c.Url, &auth.BasicAuth{Username: c.Username, Password: c.Token}, cli, logger) if err != nil { return nil, errors.Wrap(err, "creating Azure DevOps client") } - return &AzureDevOpsSource{client: client}, nil + return &AzureDevOpsSource{client: client, logger: logger}, nil } // GitserverPushConfig returns an authenticated push config used for pushing // commits to the code host. -func (s AzureDevOpsSource) GitserverPushConfig(ctx context.Context, repo *types.Repo) (*protocol.PushConfig, error) { - return GitserverPushConfig(ctx, repo, s.client.Authenticator()) +func (s *AzureDevOpsSource) GitserverPushConfig(ctx context.Context, repo *types.Repo) (*protocol.PushConfig, error) { + return GitserverPushConfig(ctx, repo, s.client.Authenticator(), s.logger) } -func (s AzureDevOpsSource) AuthenticationStrategy() AuthenticationStrategy { +func (s *AzureDevOpsSource) AuthenticationStrategy() AuthenticationStrategy { return AuthenticationStrategyUserCredential } // WithAuthenticator returns a copy of the original Source configured to use the // given authenticator, provided that authenticator type is supported by the // code host. -func (s AzureDevOpsSource) WithAuthenticator(a auth.Authenticator) (ChangesetSource, error) { +func (s *AzureDevOpsSource) WithAuthenticator(a auth.Authenticator) (ChangesetSource, error) { client, err := s.client.WithAuthenticator(a) if err != nil { return nil, err @@ -79,7 +84,7 @@ func (s AzureDevOpsSource) WithAuthenticator(a auth.Authenticator) (ChangesetSou // ValidateAuthenticator validates the currently set authenticator is usable. // Returns an error, when validating the Authenticator yielded an error. -func (s AzureDevOpsSource) ValidateAuthenticator(ctx context.Context) error { +func (s *AzureDevOpsSource) ValidateAuthenticator(ctx context.Context) error { _, err := s.client.GetAuthorizedProfile(ctx) return err } @@ -87,7 +92,7 @@ func (s AzureDevOpsSource) ValidateAuthenticator(ctx context.Context) error { // LoadChangeset loads the given Changeset from the source and updates it. If // the Changeset could not be found on the source, a ChangesetNotFoundError is // returned. -func (s AzureDevOpsSource) LoadChangeset(ctx context.Context, cs *Changeset) error { +func (s *AzureDevOpsSource) LoadChangeset(ctx context.Context, cs *Changeset) error { repo := cs.TargetRepo.Metadata.(*azuredevops.Repository) args, err := s.createCommonPullRequestArgs(*repo, *cs) if err != nil { @@ -107,19 +112,19 @@ func (s AzureDevOpsSource) LoadChangeset(ctx context.Context, cs *Changeset) err // CreateChangeset will create the Changeset on the source. If it already // exists, *Changeset will be populated and the return value will be true. -func (s AzureDevOpsSource) CreateChangeset(ctx context.Context, cs *Changeset) (bool, error) { +func (s *AzureDevOpsSource) CreateChangeset(ctx context.Context, cs *Changeset) (bool, error) { input := s.changesetToPullRequestInput(cs) return s.createChangeset(ctx, cs, input) } // CreateDraftChangeset creates the given changeset on the code host in draft mode. -func (s AzureDevOpsSource) CreateDraftChangeset(ctx context.Context, cs *Changeset) (bool, error) { +func (s *AzureDevOpsSource) CreateDraftChangeset(ctx context.Context, cs *Changeset) (bool, error) { input := s.changesetToPullRequestInput(cs) input.IsDraft = true return s.createChangeset(ctx, cs, input) } -func (s AzureDevOpsSource) createChangeset(ctx context.Context, cs *Changeset, input azuredevops.CreatePullRequestInput) (bool, error) { +func (s *AzureDevOpsSource) createChangeset(ctx context.Context, cs *Changeset, input azuredevops.CreatePullRequestInput) (bool, error) { repo := cs.TargetRepo.Metadata.(*azuredevops.Repository) org, err := repo.GetOrganization() if err != nil { @@ -144,7 +149,7 @@ func (s AzureDevOpsSource) createChangeset(ctx context.Context, cs *Changeset, i } // UndraftChangeset will update the Changeset on the source to be not in draft mode anymore. -func (s AzureDevOpsSource) UndraftChangeset(ctx context.Context, cs *Changeset) error { +func (s *AzureDevOpsSource) UndraftChangeset(ctx context.Context, cs *Changeset) error { input := s.changesetToUpdatePullRequestInput(cs, false) isDraft := false input.IsDraft = &isDraft @@ -165,7 +170,7 @@ func (s AzureDevOpsSource) UndraftChangeset(ctx context.Context, cs *Changeset) // CloseChangeset will close the Changeset on the source, where "close" // means the appropriate final state on the codehost (e.g. "abandoned" on // AzureDevOps). -func (s AzureDevOpsSource) CloseChangeset(ctx context.Context, cs *Changeset) error { +func (s *AzureDevOpsSource) CloseChangeset(ctx context.Context, cs *Changeset) error { repo := cs.TargetRepo.Metadata.(*azuredevops.Repository) args, err := s.createCommonPullRequestArgs(*repo, *cs) if err != nil { @@ -189,7 +194,7 @@ func (s AzureDevOpsSource) CloseChangeset(ctx context.Context, cs *Changeset) er } // UpdateChangeset can update Changesets. -func (s AzureDevOpsSource) UpdateChangeset(ctx context.Context, cs *Changeset) error { +func (s *AzureDevOpsSource) UpdateChangeset(ctx context.Context, cs *Changeset) error { repo := cs.TargetRepo.Metadata.(*azuredevops.Repository) args, err := s.createCommonPullRequestArgs(*repo, *cs) if err != nil { @@ -224,7 +229,7 @@ func (s AzureDevOpsSource) UpdateChangeset(ctx context.Context, cs *Changeset) e // ReopenChangeset will reopen the Changeset on the source, if it's closed. // If not, it's a noop. -func (s AzureDevOpsSource) ReopenChangeset(ctx context.Context, cs *Changeset) error { +func (s *AzureDevOpsSource) ReopenChangeset(ctx context.Context, cs *Changeset) error { deleteSourceBranch := conf.Get().BatchChangesAutoDeleteBranch input := azuredevops.PullRequestUpdateInput{ Status: &azuredevops.PullRequestStatusActive, @@ -247,7 +252,7 @@ func (s AzureDevOpsSource) ReopenChangeset(ctx context.Context, cs *Changeset) e } // CreateComment posts a comment on the Changeset. -func (s AzureDevOpsSource) CreateComment(ctx context.Context, cs *Changeset, comment string) error { +func (s *AzureDevOpsSource) CreateComment(ctx context.Context, cs *Changeset, comment string) error { repo := cs.TargetRepo.Metadata.(*azuredevops.Repository) args, err := s.createCommonPullRequestArgs(*repo, *cs) if err != nil { @@ -271,7 +276,7 @@ func (s AzureDevOpsSource) CreateComment(ctx context.Context, cs *Changeset, com // must attempt a squash merge. Otherwise, it is expected to perform a regular // merge. If the changeset cannot be merged, because it is in an unmergeable // state, ChangesetNotMergeableError must be returned. -func (s AzureDevOpsSource) MergeChangeset(ctx context.Context, cs *Changeset, squash bool) error { +func (s *AzureDevOpsSource) MergeChangeset(ctx context.Context, cs *Changeset, squash bool) error { repo := cs.TargetRepo.Metadata.(*azuredevops.Repository) args, err := s.createCommonPullRequestArgs(*repo, *cs) if err != nil { @@ -304,7 +309,7 @@ func (s AzureDevOpsSource) MergeChangeset(ctx context.Context, cs *Changeset, sq // exists and creating it if it doesn't. If namespace is not provided, the original namespace is used. // If name is not provided, the fork will be named with the default Sourcegraph convention: // "${original-namespace}-${original-name}" -func (s AzureDevOpsSource) GetFork(ctx context.Context, targetRepo *types.Repo, ns, n *string) (*types.Repo, error) { +func (s *AzureDevOpsSource) GetFork(ctx context.Context, targetRepo *types.Repo, ns, n *string) (*types.Repo, error) { tr := targetRepo.Metadata.(*azuredevops.Repository) var namespace string @@ -371,12 +376,12 @@ func (s AzureDevOpsSource) GetFork(ctx context.Context, targetRepo *types.Repo, return s.checkAndCopy(targetRepo, &fork) } -func (s AzureDevOpsSource) BuildCommitOpts(repo *types.Repo, _ *btypes.Changeset, spec *btypes.ChangesetSpec, pushOpts *protocol.PushConfig) protocol.CreateCommitFromPatchRequest { +func (s *AzureDevOpsSource) BuildCommitOpts(repo *types.Repo, _ *btypes.Changeset, spec *btypes.ChangesetSpec, pushOpts *protocol.PushConfig) protocol.CreateCommitFromPatchRequest { return BuildCommitOptsCommon(repo, spec, pushOpts) } // checkAndCopy creates a types.Repo representation of the forked repository useing the original repo (targetRepo). -func (s AzureDevOpsSource) checkAndCopy(targetRepo *types.Repo, fork *azuredevops.Repository) (*types.Repo, error) { +func (s *AzureDevOpsSource) checkAndCopy(targetRepo *types.Repo, fork *azuredevops.Repository) (*types.Repo, error) { if !fork.IsFork { return nil, errors.New("repo is not a fork") } @@ -392,7 +397,7 @@ func (s AzureDevOpsSource) checkAndCopy(targetRepo *types.Repo, fork *azuredevop return forkRepo, nil } -func (s AzureDevOpsSource) annotatePullRequest(ctx context.Context, repo *azuredevops.Repository, pr *azuredevops.PullRequest) (*adobatches.AnnotatedPullRequest, error) { +func (s *AzureDevOpsSource) annotatePullRequest(ctx context.Context, repo *azuredevops.Repository, pr *azuredevops.PullRequest) (*adobatches.AnnotatedPullRequest, error) { org, err := repo.GetOrganization() if err != nil { return nil, err @@ -419,7 +424,7 @@ func (s AzureDevOpsSource) annotatePullRequest(ctx context.Context, repo *azured }, nil } -func (s AzureDevOpsSource) setChangesetMetadata(ctx context.Context, repo *azuredevops.Repository, pr *azuredevops.PullRequest, cs *Changeset) error { +func (s *AzureDevOpsSource) setChangesetMetadata(ctx context.Context, repo *azuredevops.Repository, pr *azuredevops.PullRequest, cs *Changeset) error { apr, err := s.annotatePullRequest(ctx, repo, pr) if err != nil { return errors.Wrap(err, "annotating pull request") @@ -432,7 +437,7 @@ func (s AzureDevOpsSource) setChangesetMetadata(ctx context.Context, repo *azure return nil } -func (s AzureDevOpsSource) changesetToPullRequestInput(cs *Changeset) azuredevops.CreatePullRequestInput { +func (s *AzureDevOpsSource) changesetToPullRequestInput(cs *Changeset) azuredevops.CreatePullRequestInput { deleteSourceBranch := conf.Get().BatchChangesAutoDeleteBranch input := azuredevops.CreatePullRequestInput{ Title: cs.Title, @@ -454,7 +459,7 @@ func (s AzureDevOpsSource) changesetToPullRequestInput(cs *Changeset) azuredevop return input } -func (s AzureDevOpsSource) changesetToUpdatePullRequestInput(cs *Changeset, targetRefChanged bool) azuredevops.PullRequestUpdateInput { +func (s *AzureDevOpsSource) changesetToUpdatePullRequestInput(cs *Changeset, targetRefChanged bool) azuredevops.PullRequestUpdateInput { targetRef := gitdomain.EnsureRefPrefix(cs.BaseRef) if targetRefChanged { return azuredevops.PullRequestUpdateInput{ @@ -472,7 +477,7 @@ func (s AzureDevOpsSource) changesetToUpdatePullRequestInput(cs *Changeset, targ } } -func (s AzureDevOpsSource) createCommonPullRequestArgs(repo azuredevops.Repository, cs Changeset) (azuredevops.PullRequestCommonArgs, error) { +func (s *AzureDevOpsSource) createCommonPullRequestArgs(repo azuredevops.Repository, cs Changeset) (azuredevops.PullRequestCommonArgs, error) { org, err := repo.GetOrganization() if err != nil { return azuredevops.PullRequestCommonArgs{}, errors.Wrap(err, "getting Azure DevOps organization from project") diff --git a/internal/batches/sources/azuredevops_test.go b/internal/batches/sources/azuredevops_test.go index 9777477ca1e56..def68dbe81a29 100644 --- a/internal/batches/sources/azuredevops_test.go +++ b/internal/batches/sources/azuredevops_test.go @@ -6,6 +6,7 @@ import ( "strconv" "testing" + "github.com/sourcegraph/log/logtest" "github.com/stretchr/testify/assert" "github.com/sourcegraph/sourcegraph/internal/api" @@ -39,7 +40,7 @@ func TestAzureDevOpsSource_GitserverPushConfig(t *testing.T) { // So, cue the boilerplate: au := auth.BasicAuth{Username: "user", Password: "pass"} - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) client.AuthenticatorFunc.SetDefaultReturn(&au) repo := &types.Repo{ @@ -73,7 +74,7 @@ func TestAzureDevOpsSource_WithAuthenticator(t *testing.T) { t.Run("supports BasicAuth", func(t *testing.T) { newClient := NewStrictMockAzureDevOpsClient() au := &auth.BasicAuth{} - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) client.WithAuthenticatorFunc.SetDefaultHook(func(a auth.Authenticator) (azuredevops.Client, error) { assert.Same(t, au, a) return newClient, nil @@ -93,7 +94,7 @@ func TestAzureDevOpsSource_ValidateAuthenticator(t *testing.T) { "error": errors.New("error"), } { t.Run(name, func(t *testing.T) { - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) client.GetAuthorizedProfileFunc.SetDefaultReturn(azuredevops.Profile{}, want) assert.Equal(t, want, s.ValidateAuthenticator(ctx)) @@ -106,7 +107,7 @@ func TestAzureDevOpsSource_LoadChangeset(t *testing.T) { t.Run("error getting pull request", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) want := errors.New("error") client.GetPullRequestFunc.SetDefaultHook(func(ctx context.Context, r azuredevops.PullRequestCommonArgs) (azuredevops.PullRequest, error) { assert.Equal(t, testCommonPullRequestArgs, r) @@ -120,7 +121,7 @@ func TestAzureDevOpsSource_LoadChangeset(t *testing.T) { t.Run("pull request not found", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) client.GetPullRequestFunc.SetDefaultHook(func(ctx context.Context, r azuredevops.PullRequestCommonArgs) (azuredevops.PullRequest, error) { assert.Equal(t, testCommonPullRequestArgs, r) return azuredevops.PullRequest{}, ¬FoundError{} @@ -135,7 +136,7 @@ func TestAzureDevOpsSource_LoadChangeset(t *testing.T) { t.Run("error setting changeset metadata", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) want := mockAzureDevOpsAnnotatePullRequestError(client) pr := mockAzureDevOpsPullRequest(&testRepository) @@ -151,7 +152,7 @@ func TestAzureDevOpsSource_LoadChangeset(t *testing.T) { t.Run("success", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) mockAzureDevOpsAnnotatePullRequestSuccess(client) pr := mockAzureDevOpsPullRequest(&testRepository) @@ -171,7 +172,7 @@ func TestAzureDevOpsSource_CreateChangeset(t *testing.T) { t.Run("error creating pull request", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) want := errors.New("error") client.CreatePullRequestFunc.SetDefaultHook(func(ctx context.Context, r azuredevops.OrgProjectRepoArgs, pri azuredevops.CreatePullRequestInput) (azuredevops.PullRequest, error) { @@ -188,7 +189,7 @@ func TestAzureDevOpsSource_CreateChangeset(t *testing.T) { t.Run("error setting changeset metadata", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) want := mockAzureDevOpsAnnotatePullRequestError(client) pr := mockAzureDevOpsPullRequest(&testRepository) @@ -206,7 +207,7 @@ func TestAzureDevOpsSource_CreateChangeset(t *testing.T) { t.Run("success", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) mockAzureDevOpsAnnotatePullRequestSuccess(client) pr := mockAzureDevOpsPullRequest(&testRepository) @@ -225,7 +226,7 @@ func TestAzureDevOpsSource_CreateChangeset(t *testing.T) { t.Run("success with fork", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) mockAzureDevOpsAnnotatePullRequestSuccess(client) fork := &azuredevops.Repository{ @@ -256,7 +257,7 @@ func TestAzureDevOpsSource_CreateDraftChangeset(t *testing.T) { t.Run("error creating pull request", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) want := errors.New("error") client.CreatePullRequestFunc.SetDefaultHook(func(ctx context.Context, r azuredevops.OrgProjectRepoArgs, pri azuredevops.CreatePullRequestInput) (azuredevops.PullRequest, error) { @@ -273,7 +274,7 @@ func TestAzureDevOpsSource_CreateDraftChangeset(t *testing.T) { t.Run("error setting changeset metadata", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) want := mockAzureDevOpsAnnotatePullRequestError(client) pr := mockAzureDevOpsPullRequest(&testRepository) @@ -291,7 +292,7 @@ func TestAzureDevOpsSource_CreateDraftChangeset(t *testing.T) { t.Run("success", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) mockAzureDevOpsAnnotatePullRequestSuccess(client) pr := mockAzureDevOpsPullRequest(&testRepository) @@ -311,7 +312,7 @@ func TestAzureDevOpsSource_CreateDraftChangeset(t *testing.T) { t.Run("success with fork", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) mockAzureDevOpsAnnotatePullRequestSuccess(client) fork := &azuredevops.Repository{ @@ -343,7 +344,7 @@ func TestAzureDevOpsSource_CloseChangeset(t *testing.T) { t.Run("error declining pull request", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) pr := mockAzureDevOpsPullRequest(&testRepository) want := errors.New("error") @@ -360,7 +361,7 @@ func TestAzureDevOpsSource_CloseChangeset(t *testing.T) { t.Run("error setting changeset metadata", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) want := mockAzureDevOpsAnnotatePullRequestError(client) pr := mockAzureDevOpsPullRequest(&testRepository) @@ -377,7 +378,7 @@ func TestAzureDevOpsSource_CloseChangeset(t *testing.T) { t.Run("success", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) mockAzureDevOpsAnnotatePullRequestSuccess(client) pr := mockAzureDevOpsPullRequest(&testRepository) @@ -398,7 +399,7 @@ func TestAzureDevOpsSource_UpdateChangeset(t *testing.T) { t.Run("error getting pull request", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) want := errors.New("error") client.GetPullRequestFunc.SetDefaultHook(func(ctx context.Context, r azuredevops.PullRequestCommonArgs) (azuredevops.PullRequest, error) { assert.Equal(t, testCommonPullRequestArgs, r) @@ -412,7 +413,7 @@ func TestAzureDevOpsSource_UpdateChangeset(t *testing.T) { t.Run("error updating pull request", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) want := errors.New("error") pr := mockAzureDevOpsPullRequest(&testRepository) client.GetPullRequestFunc.SetDefaultHook(func(ctx context.Context, r azuredevops.PullRequestCommonArgs) (azuredevops.PullRequest, error) { @@ -433,7 +434,7 @@ func TestAzureDevOpsSource_UpdateChangeset(t *testing.T) { t.Run("error setting changeset metadata", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) want := mockAzureDevOpsAnnotatePullRequestError(client) pr := mockAzureDevOpsPullRequest(&testRepository) @@ -455,7 +456,7 @@ func TestAzureDevOpsSource_UpdateChangeset(t *testing.T) { t.Run("success", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) mockAzureDevOpsAnnotatePullRequestSuccess(client) pr := mockAzureDevOpsPullRequest(&testRepository) @@ -481,7 +482,7 @@ func TestAzureDevOpsSource_UndraftChangeset(t *testing.T) { t.Run("error updating pull request", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) want := errors.New("error") pr := mockAzureDevOpsPullRequest(&testRepository) client.UpdatePullRequestFunc.SetDefaultHook(func(ctx context.Context, r azuredevops.PullRequestCommonArgs, pri azuredevops.PullRequestUpdateInput) (azuredevops.PullRequest, error) { @@ -498,7 +499,7 @@ func TestAzureDevOpsSource_UndraftChangeset(t *testing.T) { t.Run("error setting changeset metadata", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) want := mockAzureDevOpsAnnotatePullRequestError(client) pr := mockAzureDevOpsPullRequest(&testRepository) @@ -516,7 +517,7 @@ func TestAzureDevOpsSource_UndraftChangeset(t *testing.T) { t.Run("success", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) mockAzureDevOpsAnnotatePullRequestSuccess(client) pr := mockAzureDevOpsPullRequest(&testRepository) @@ -539,7 +540,7 @@ func TestAzureDevOpsSource_CreateComment(t *testing.T) { t.Run("error creating comment", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) pr := mockAzureDevOpsPullRequest(&testRepository) want := errors.New("error") @@ -557,7 +558,7 @@ func TestAzureDevOpsSource_CreateComment(t *testing.T) { t.Run("success", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) pr := mockAzureDevOpsPullRequest(&testRepository) client.CreatePullRequestCommentThreadFunc.SetDefaultHook(func(ctx context.Context, r azuredevops.PullRequestCommonArgs, ci azuredevops.PullRequestCommentInput) (azuredevops.PullRequestCommentResponse, error) { @@ -577,7 +578,7 @@ func TestAzureDevOpsSource_MergeChangeset(t *testing.T) { t.Run("error merging pull request", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) pr := mockAzureDevOpsPullRequest(&testRepository) want := errors.New("error") @@ -597,7 +598,7 @@ func TestAzureDevOpsSource_MergeChangeset(t *testing.T) { t.Run("pull request not found", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) pr := mockAzureDevOpsPullRequest(&testRepository) want := ¬FoundError{} @@ -615,7 +616,7 @@ func TestAzureDevOpsSource_MergeChangeset(t *testing.T) { t.Run("error setting changeset metadata", func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) want := mockAzureDevOpsAnnotatePullRequestError(client) pr := mockAzureDevOpsPullRequest(&testRepository) @@ -642,7 +643,7 @@ func TestAzureDevOpsSource_MergeChangeset(t *testing.T) { } { t.Run(name, func(t *testing.T) { cs, _ := mockAzureDevOpsChangeset() - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) mockAzureDevOpsAnnotatePullRequestSuccess(client) pr := mockAzureDevOpsPullRequest(&testRepository) @@ -703,7 +704,7 @@ func TestAzureDevOpsSource_GetFork(t *testing.T) { } t.Run("error checking for repo", func(t *testing.T) { - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) want := errors.New("error") client.GetRepoFunc.SetDefaultHook(func(ctx context.Context, a azuredevops.OrgProjectRepoArgs) (azuredevops.Repository, error) { @@ -718,7 +719,7 @@ func TestAzureDevOpsSource_GetFork(t *testing.T) { }) t.Run("forked repo already exists", func(t *testing.T) { - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) client.GetRepoFunc.SetDefaultHook(func(ctx context.Context, a azuredevops.OrgProjectRepoArgs) (azuredevops.Repository, error) { assert.Equal(t, args, a) @@ -734,7 +735,7 @@ func TestAzureDevOpsSource_GetFork(t *testing.T) { }) t.Run("get project error", func(t *testing.T) { - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) client.GetRepoFunc.SetDefaultHook(func(ctx context.Context, a azuredevops.OrgProjectRepoArgs) (azuredevops.Repository, error) { assert.Equal(t, args, a) @@ -754,7 +755,7 @@ func TestAzureDevOpsSource_GetFork(t *testing.T) { }) t.Run("fork error", func(t *testing.T) { - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) client.GetRepoFunc.SetDefaultHook(func(ctx context.Context, a azuredevops.OrgProjectRepoArgs) (azuredevops.Repository, error) { assert.Equal(t, args, a) @@ -781,7 +782,7 @@ func TestAzureDevOpsSource_GetFork(t *testing.T) { }) t.Run("success with default namespace, name", func(t *testing.T) { - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) client.GetRepoFunc.SetDefaultHook(func(ctx context.Context, a azuredevops.OrgProjectRepoArgs) (azuredevops.Repository, error) { argsNew := args @@ -797,7 +798,7 @@ func TestAzureDevOpsSource_GetFork(t *testing.T) { }) t.Run("success with default name", func(t *testing.T) { - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) client.GetRepoFunc.SetDefaultHook(func(ctx context.Context, a azuredevops.OrgProjectRepoArgs) (azuredevops.Repository, error) { assert.Equal(t, args, a) @@ -825,7 +826,7 @@ func TestAzureDevOpsSource_GetFork(t *testing.T) { }) t.Run("success with set namespace, name", func(t *testing.T) { - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) client.GetRepoFunc.SetDefaultHook(func(ctx context.Context, a azuredevops.OrgProjectRepoArgs) (azuredevops.Repository, error) { newArgs := args @@ -865,7 +866,7 @@ func TestAzureDevOpsSource_annotatePullRequest(t *testing.T) { ctx := context.Background() t.Run("error getting all statuses", func(t *testing.T) { - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) pr := mockAzureDevOpsPullRequest(&testRepository) want := errors.New("error") @@ -881,7 +882,7 @@ func TestAzureDevOpsSource_annotatePullRequest(t *testing.T) { }) t.Run("success", func(t *testing.T) { - s, client := mockAzureDevOpsSource() + s, client := mockAzureDevOpsSource(t) pr := mockAzureDevOpsPullRequest(&testRepository) want := []*azuredevops.PullRequestBuildStatus{ @@ -960,9 +961,9 @@ func annotateChangesetWithPullRequest(cs *Changeset, pr *azuredevops.PullRequest } } -func mockAzureDevOpsSource() (*AzureDevOpsSource, *MockAzureDevOpsClient) { +func mockAzureDevOpsSource(t *testing.T) (*AzureDevOpsSource, *MockAzureDevOpsClient) { client := NewStrictMockAzureDevOpsClient() - s := &AzureDevOpsSource{client: client} + s := &AzureDevOpsSource{client: client, logger: logtest.Scoped(t)} return s, client } diff --git a/internal/batches/sources/bitbucketcloud.go b/internal/batches/sources/bitbucketcloud.go index 19d54f102dc28..125320851c00f 100644 --- a/internal/batches/sources/bitbucketcloud.go +++ b/internal/batches/sources/bitbucketcloud.go @@ -4,6 +4,8 @@ import ( "context" "strconv" + "github.com/sourcegraph/log" + bbcs "github.com/sourcegraph/sourcegraph/internal/batches/sources/bitbucketcloud" btypes "github.com/sourcegraph/sourcegraph/internal/batches/types" "github.com/sourcegraph/sourcegraph/internal/conf" @@ -21,11 +23,13 @@ import ( type BitbucketCloudSource struct { client bitbucketcloud.Client + logger log.Logger } -var _ ForkableChangesetSource = BitbucketCloudSource{} +var _ ForkableChangesetSource = &BitbucketCloudSource{} -func NewBitbucketCloudSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory) (*BitbucketCloudSource, error) { +func NewBitbucketCloudSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory, logger log.Logger) (*BitbucketCloudSource, error) { + logger = logger.Scoped("BitbucketCloudSource") rawConfig, err := svc.Config.Decrypt(ctx) if err != nil { return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err) @@ -36,7 +40,7 @@ func NewBitbucketCloudSource(ctx context.Context, svc *types.ExternalService, cf } if cf == nil { - cf = httpcli.ExternalClientFactory + cf = httpcli.ExternalClientFactory(logger) } // No options to provide here, since Bitbucket Cloud doesn't support custom @@ -46,28 +50,28 @@ func NewBitbucketCloudSource(ctx context.Context, svc *types.ExternalService, cf return nil, errors.Wrap(err, "creating external client") } - client, err := bitbucketcloud.NewClient(svc.URN(), &c, cli) + client, err := bitbucketcloud.NewClient(svc.URN(), &c, cli, logger) if err != nil { return nil, errors.Wrap(err, "creating Bitbucket Cloud client") } - return &BitbucketCloudSource{client: client}, nil + return &BitbucketCloudSource{client: client, logger: logger}, nil } -func (s BitbucketCloudSource) AuthenticationStrategy() AuthenticationStrategy { +func (s *BitbucketCloudSource) AuthenticationStrategy() AuthenticationStrategy { return AuthenticationStrategyUserCredential } // GitserverPushConfig returns an authenticated push config used for pushing // commits to the code host. -func (s BitbucketCloudSource) GitserverPushConfig(ctx context.Context, repo *types.Repo) (*protocol.PushConfig, error) { - return GitserverPushConfig(ctx, repo, s.client.Authenticator()) +func (s *BitbucketCloudSource) GitserverPushConfig(ctx context.Context, repo *types.Repo) (*protocol.PushConfig, error) { + return GitserverPushConfig(ctx, repo, s.client.Authenticator(), s.logger) } // WithAuthenticator returns a copy of the original Source configured to use the // given authenticator, provided that authenticator type is supported by the // code host. -func (s BitbucketCloudSource) WithAuthenticator(a auth.Authenticator) (ChangesetSource, error) { +func (s *BitbucketCloudSource) WithAuthenticator(a auth.Authenticator) (ChangesetSource, error) { switch a.(type) { case *auth.BasicAuth, *auth.BasicAuthWithSSH: @@ -82,14 +86,14 @@ func (s BitbucketCloudSource) WithAuthenticator(a auth.Authenticator) (Changeset // ValidateAuthenticator validates the currently set authenticator is usable. // Returns an error, when validating the Authenticator yielded an error. -func (s BitbucketCloudSource) ValidateAuthenticator(ctx context.Context) error { +func (s *BitbucketCloudSource) ValidateAuthenticator(ctx context.Context) error { return s.client.Ping(ctx) } // LoadChangeset loads the given Changeset from the source and updates it. If // the Changeset could not be found on the source, a ChangesetNotFoundError is // returned. -func (s BitbucketCloudSource) LoadChangeset(ctx context.Context, cs *Changeset) error { +func (s *BitbucketCloudSource) LoadChangeset(ctx context.Context, cs *Changeset) error { repo := cs.TargetRepo.Metadata.(*bitbucketcloud.Repo) number, err := strconv.Atoi(cs.ExternalID) if err != nil { @@ -109,7 +113,7 @@ func (s BitbucketCloudSource) LoadChangeset(ctx context.Context, cs *Changeset) // CreateChangeset will create the Changeset on the source. If it already // exists, *Changeset will be populated and the return value will be true. -func (s BitbucketCloudSource) CreateChangeset(ctx context.Context, cs *Changeset) (bool, error) { +func (s *BitbucketCloudSource) CreateChangeset(ctx context.Context, cs *Changeset) (bool, error) { opts := s.changesetToPullRequestInput(cs) targetRepo := cs.TargetRepo.Metadata.(*bitbucketcloud.Repo) @@ -132,7 +136,7 @@ func (s BitbucketCloudSource) CreateChangeset(ctx context.Context, cs *Changeset // CloseChangeset will close the Changeset on the source, where "close" // means the appropriate final state on the codehost (e.g. "declined" on // Bitbucket Server). -func (s BitbucketCloudSource) CloseChangeset(ctx context.Context, cs *Changeset) error { +func (s *BitbucketCloudSource) CloseChangeset(ctx context.Context, cs *Changeset) error { repo := cs.TargetRepo.Metadata.(*bitbucketcloud.Repo) pr := cs.Metadata.(*bbcs.AnnotatedPullRequest) updated, err := s.client.DeclinePullRequest(ctx, repo, pr.ID) @@ -144,7 +148,7 @@ func (s BitbucketCloudSource) CloseChangeset(ctx context.Context, cs *Changeset) } // UpdateChangeset can update Changesets. -func (s BitbucketCloudSource) UpdateChangeset(ctx context.Context, cs *Changeset) error { +func (s *BitbucketCloudSource) UpdateChangeset(ctx context.Context, cs *Changeset) error { opts := s.changesetToPullRequestInput(cs) targetRepo := cs.TargetRepo.Metadata.(*bitbucketcloud.Repo) @@ -168,7 +172,7 @@ func (s BitbucketCloudSource) UpdateChangeset(ctx context.Context, cs *Changeset // ReopenChangeset will reopen the Changeset on the source, if it's closed. // If not, it's a noop. -func (s BitbucketCloudSource) ReopenChangeset(ctx context.Context, cs *Changeset) error { +func (s *BitbucketCloudSource) ReopenChangeset(ctx context.Context, cs *Changeset) error { // Bitbucket Cloud is a bit special, and can't reopen a declined PR under // any circumstances. (See https://jira.atlassian.com/browse/BCLOUD-4954 for // more details.) @@ -185,7 +189,7 @@ func (s BitbucketCloudSource) ReopenChangeset(ctx context.Context, cs *Changeset } // CreateComment posts a comment on the Changeset. -func (s BitbucketCloudSource) CreateComment(ctx context.Context, cs *Changeset, comment string) error { +func (s *BitbucketCloudSource) CreateComment(ctx context.Context, cs *Changeset, comment string) error { repo := cs.TargetRepo.Metadata.(*bitbucketcloud.Repo) pr := cs.Metadata.(*bbcs.AnnotatedPullRequest) @@ -200,7 +204,7 @@ func (s BitbucketCloudSource) CreateComment(ctx context.Context, cs *Changeset, // must attempt a squash merge. Otherwise, it is expected to perform a regular // merge. If the changeset cannot be merged, because it is in an unmergeable // state, ChangesetNotMergeableError must be returned. -func (s BitbucketCloudSource) MergeChangeset(ctx context.Context, cs *Changeset, squash bool) error { +func (s *BitbucketCloudSource) MergeChangeset(ctx context.Context, cs *Changeset, squash bool) error { repo := cs.TargetRepo.Metadata.(*bitbucketcloud.Repo) pr := cs.Metadata.(*bbcs.AnnotatedPullRequest) @@ -227,7 +231,7 @@ func (s BitbucketCloudSource) MergeChangeset(ctx context.Context, cs *Changeset, // exists and creating it if it doesn't. If namespace is not provided, the fork will be in // the currently authenticated user's namespace. If name is not provided, the fork will be // named with the default Sourcegraph convention: "${original-namespace}-${original-name}" -func (s BitbucketCloudSource) GetFork(ctx context.Context, targetRepo *types.Repo, ns, n *string) (*types.Repo, error) { +func (s *BitbucketCloudSource) GetFork(ctx context.Context, targetRepo *types.Repo, ns, n *string) (*types.Repo, error) { var namespace string if ns != nil { namespace = *ns @@ -271,11 +275,11 @@ func (s BitbucketCloudSource) GetFork(ctx context.Context, targetRepo *types.Rep return s.checkAndCopy(targetRepo, fork) } -func (s BitbucketCloudSource) BuildCommitOpts(repo *types.Repo, _ *btypes.Changeset, spec *btypes.ChangesetSpec, pushOpts *protocol.PushConfig) protocol.CreateCommitFromPatchRequest { +func (s *BitbucketCloudSource) BuildCommitOpts(repo *types.Repo, _ *btypes.Changeset, spec *btypes.ChangesetSpec, pushOpts *protocol.PushConfig) protocol.CreateCommitFromPatchRequest { return BuildCommitOptsCommon(repo, spec, pushOpts) } -func (s BitbucketCloudSource) checkAndCopy(targetRepo *types.Repo, fork *bitbucketcloud.Repo) (*types.Repo, error) { +func (s *BitbucketCloudSource) checkAndCopy(targetRepo *types.Repo, fork *bitbucketcloud.Repo) (*types.Repo, error) { tr := targetRepo.Metadata.(*bitbucketcloud.Repo) if fork.Parent == nil { @@ -294,7 +298,7 @@ func (s BitbucketCloudSource) checkAndCopy(targetRepo *types.Repo, fork *bitbuck return forkRepo, nil } -func (s BitbucketCloudSource) annotatePullRequest(ctx context.Context, repo *bitbucketcloud.Repo, pr *bitbucketcloud.PullRequest) (*bbcs.AnnotatedPullRequest, error) { +func (s *BitbucketCloudSource) annotatePullRequest(ctx context.Context, repo *bitbucketcloud.Repo, pr *bitbucketcloud.PullRequest) (*bbcs.AnnotatedPullRequest, error) { srs, err := s.client.GetPullRequestStatuses(repo, pr.ID) if err != nil { return nil, errors.Wrap(err, "getting pull request statuses") @@ -315,7 +319,7 @@ func (s BitbucketCloudSource) annotatePullRequest(ctx context.Context, repo *bit }, nil } -func (s BitbucketCloudSource) setChangesetMetadata(ctx context.Context, repo *bitbucketcloud.Repo, pr *bitbucketcloud.PullRequest, cs *Changeset) error { +func (s *BitbucketCloudSource) setChangesetMetadata(ctx context.Context, repo *bitbucketcloud.Repo, pr *bitbucketcloud.PullRequest, cs *Changeset) error { apr, err := s.annotatePullRequest(ctx, repo, pr) if err != nil { return errors.Wrap(err, "annotating pull request") @@ -328,7 +332,7 @@ func (s BitbucketCloudSource) setChangesetMetadata(ctx context.Context, repo *bi return nil } -func (s BitbucketCloudSource) changesetToPullRequestInput(cs *Changeset) bitbucketcloud.PullRequestInput { +func (s *BitbucketCloudSource) changesetToPullRequestInput(cs *Changeset) bitbucketcloud.PullRequestInput { destBranch := gitdomain.AbbreviateRef(cs.BaseRef) closeSourceBranch := conf.Get().BatchChangesAutoDeleteBranch diff --git a/internal/batches/sources/bitbucketcloud_test.go b/internal/batches/sources/bitbucketcloud_test.go index 5d66052805327..ff68cf8ff77ef 100644 --- a/internal/batches/sources/bitbucketcloud_test.go +++ b/internal/batches/sources/bitbucketcloud_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/sourcegraph/log/logtest" "github.com/stretchr/testify/assert" "github.com/sourcegraph/sourcegraph/internal/api" @@ -33,7 +34,7 @@ func TestNewBitbucketCloudSource(t *testing.T) { ctx := context.Background() s, err := NewBitbucketCloudSource(ctx, &types.ExternalService{ Config: extsvc.NewUnencryptedConfig(input), - }, nil) + }, nil, logtest.Scoped(t)) assert.Nil(t, s) assert.NotNil(t, err) }) @@ -42,7 +43,7 @@ func TestNewBitbucketCloudSource(t *testing.T) { t.Run("valid", func(t *testing.T) { ctx := context.Background() - s, err := NewBitbucketCloudSource(ctx, &types.ExternalService{Config: extsvc.NewEmptyConfig()}, nil) + s, err := NewBitbucketCloudSource(ctx, &types.ExternalService{Config: extsvc.NewEmptyConfig()}, nil, logtest.Scoped(t)) assert.NotNil(t, s) assert.Nil(t, err) }) @@ -59,7 +60,7 @@ func TestBitbucketCloudSource_GitserverPushConfig(t *testing.T) { au := auth.BasicAuthWithSSH{ BasicAuth: auth.BasicAuth{Username: "user", Password: "pass"}, } - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) client.AuthenticatorFunc.SetDefaultReturn(&au) repo := &types.Repo{ @@ -93,7 +94,7 @@ func TestBitbucketCloudSource_GitserverPushConfig(t *testing.T) { func TestBitbucketCloudSource_WithAuthenticator(t *testing.T) { t.Run("unsupported types", func(t *testing.T) { - s, _ := mockBitbucketCloudSource() + s, _ := mockBitbucketCloudSource(t) for _, au := range []auth.Authenticator{ &auth.OAuthBearerToken{}, @@ -117,7 +118,7 @@ func TestBitbucketCloudSource_WithAuthenticator(t *testing.T) { t.Run(fmt.Sprintf("%T", au), func(t *testing.T) { newClient := NewStrictMockBitbucketCloudClient() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) client.WithAuthenticatorFunc.SetDefaultHook(func(a auth.Authenticator) bitbucketcloud.Client { assert.Same(t, au, a) return newClient @@ -139,7 +140,7 @@ func TestBitbucketCloudSource_ValidateAuthenticator(t *testing.T) { "error": errors.New("error"), } { t.Run(name, func(t *testing.T) { - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) client.PingFunc.SetDefaultReturn(want) assert.Equal(t, want, s.ValidateAuthenticator(ctx)) @@ -151,7 +152,7 @@ func TestBitbucketCloudSource_LoadChangeset(t *testing.T) { ctx := context.Background() t.Run("invalid external ID", func(t *testing.T) { - s, _ := mockBitbucketCloudSource() + s, _ := mockBitbucketCloudSource(t) cs, _, _ := mockBitbucketCloudChangeset() cs.ExternalID = "not a number" @@ -162,7 +163,7 @@ func TestBitbucketCloudSource_LoadChangeset(t *testing.T) { t.Run("error getting pull request", func(t *testing.T) { cs, repo, _ := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) want := errors.New("error") client.GetPullRequestFunc.SetDefaultHook(func(ctx context.Context, r *bitbucketcloud.Repo, i int64) (*bitbucketcloud.PullRequest, error) { assert.Same(t, repo.Metadata, r) @@ -177,7 +178,7 @@ func TestBitbucketCloudSource_LoadChangeset(t *testing.T) { t.Run("pull request not found", func(t *testing.T) { cs, repo, _ := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) client.GetPullRequestFunc.SetDefaultHook(func(ctx context.Context, r *bitbucketcloud.Repo, i int64) (*bitbucketcloud.PullRequest, error) { assert.Same(t, repo.Metadata, r) assert.EqualValues(t, 42, i) @@ -193,7 +194,7 @@ func TestBitbucketCloudSource_LoadChangeset(t *testing.T) { t.Run("error setting changeset metadata", func(t *testing.T) { cs, repo, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) want := mockAnnotatePullRequestError(client) pr := mockBitbucketCloudPullRequest(bbRepo) @@ -210,7 +211,7 @@ func TestBitbucketCloudSource_LoadChangeset(t *testing.T) { t.Run("success", func(t *testing.T) { cs, repo, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) mockAnnotatePullRequestSuccess(client) pr := mockBitbucketCloudPullRequest(bbRepo) @@ -231,7 +232,7 @@ func TestBitbucketCloudSource_CreateChangeset(t *testing.T) { t.Run("error creating pull request", func(t *testing.T) { cs, repo, _ := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) want := errors.New("error") client.CreatePullRequestFunc.SetDefaultHook(func(ctx context.Context, r *bitbucketcloud.Repo, pri bitbucketcloud.PullRequestInput) (*bitbucketcloud.PullRequest, error) { @@ -248,7 +249,7 @@ func TestBitbucketCloudSource_CreateChangeset(t *testing.T) { t.Run("error setting changeset metadata", func(t *testing.T) { cs, repo, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) want := mockAnnotatePullRequestError(client) pr := mockBitbucketCloudPullRequest(bbRepo) @@ -266,7 +267,7 @@ func TestBitbucketCloudSource_CreateChangeset(t *testing.T) { t.Run("success", func(t *testing.T) { cs, repo, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) mockAnnotatePullRequestSuccess(client) pr := mockBitbucketCloudPullRequest(bbRepo) @@ -285,7 +286,7 @@ func TestBitbucketCloudSource_CreateChangeset(t *testing.T) { t.Run("success with fork", func(t *testing.T) { cs, repo, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) mockAnnotatePullRequestSuccess(client) fork := &bitbucketcloud.Repo{ @@ -317,7 +318,7 @@ func TestBitbucketCloudSource_CloseChangeset(t *testing.T) { t.Run("error declining pull request", func(t *testing.T) { cs, _, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) pr := mockBitbucketCloudPullRequest(bbRepo) want := errors.New("error") @@ -335,7 +336,7 @@ func TestBitbucketCloudSource_CloseChangeset(t *testing.T) { t.Run("error setting changeset metadata", func(t *testing.T) { cs, _, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) want := mockAnnotatePullRequestError(client) pr := mockBitbucketCloudPullRequest(bbRepo) @@ -353,7 +354,7 @@ func TestBitbucketCloudSource_CloseChangeset(t *testing.T) { t.Run("success", func(t *testing.T) { cs, _, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) mockAnnotatePullRequestSuccess(client) pr := mockBitbucketCloudPullRequest(bbRepo) @@ -375,7 +376,7 @@ func TestBitbucketCloudSource_UpdateChangeset(t *testing.T) { t.Run("error updating pull request", func(t *testing.T) { cs, _, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) pr := mockBitbucketCloudPullRequest(bbRepo) want := errors.New("error") @@ -398,7 +399,7 @@ func TestBitbucketCloudSource_UpdateChangeset(t *testing.T) { t.Run("error setting changeset metadata", func(t *testing.T) { cs, _, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) want := mockAnnotatePullRequestError(client) pr := mockBitbucketCloudPullRequest(bbRepo) @@ -421,7 +422,7 @@ func TestBitbucketCloudSource_UpdateChangeset(t *testing.T) { t.Run("success", func(t *testing.T) { cs, _, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) mockAnnotatePullRequestSuccess(client) pr := mockBitbucketCloudPullRequest(bbRepo) @@ -448,7 +449,7 @@ func TestBitbucketCloudSource_CreateComment(t *testing.T) { t.Run("error creating comment", func(t *testing.T) { cs, _, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) pr := mockBitbucketCloudPullRequest(bbRepo) want := errors.New("error") @@ -467,7 +468,7 @@ func TestBitbucketCloudSource_CreateComment(t *testing.T) { t.Run("success", func(t *testing.T) { cs, _, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) pr := mockBitbucketCloudPullRequest(bbRepo) client.CreatePullRequestCommentFunc.SetDefaultHook(func(ctx context.Context, r *bitbucketcloud.Repo, i int64, ci bitbucketcloud.CommentInput) (*bitbucketcloud.Comment, error) { @@ -488,7 +489,7 @@ func TestBitbucketCloudSource_MergeChangeset(t *testing.T) { t.Run("error merging pull request", func(t *testing.T) { cs, _, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) pr := mockBitbucketCloudPullRequest(bbRepo) want := errors.New("error") @@ -509,7 +510,7 @@ func TestBitbucketCloudSource_MergeChangeset(t *testing.T) { t.Run("pull request not found", func(t *testing.T) { cs, _, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) pr := mockBitbucketCloudPullRequest(bbRepo) want := ¬FoundError{} @@ -528,7 +529,7 @@ func TestBitbucketCloudSource_MergeChangeset(t *testing.T) { t.Run("error setting changeset metadata", func(t *testing.T) { cs, _, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) want := mockAnnotatePullRequestError(client) pr := mockBitbucketCloudPullRequest(bbRepo) @@ -556,7 +557,7 @@ func TestBitbucketCloudSource_MergeChangeset(t *testing.T) { } { t.Run(name, func(t *testing.T) { cs, _, bbRepo := mockBitbucketCloudChangeset() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) mockAnnotatePullRequestSuccess(client) pr := mockBitbucketCloudPullRequest(bbRepo) @@ -602,7 +603,7 @@ func TestBitbucketCloudSource_GetFork(t *testing.T) { } t.Run("error checking for repo", func(t *testing.T) { - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) want := errors.New("error") client.RepoFunc.SetDefaultHook(func(ctx context.Context, namespace, slug string) (*bitbucketcloud.Repo, error) { @@ -618,7 +619,7 @@ func TestBitbucketCloudSource_GetFork(t *testing.T) { }) t.Run("forked repo already exists", func(t *testing.T) { - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) client.RepoFunc.SetDefaultHook(func(ctx context.Context, namespace, slug string) (*bitbucketcloud.Repo, error) { assert.Equal(t, "fork", namespace) @@ -635,7 +636,7 @@ func TestBitbucketCloudSource_GetFork(t *testing.T) { }) t.Run("fork error", func(t *testing.T) { - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) client.RepoFunc.SetDefaultHook(func(ctx context.Context, namespace, slug string) (*bitbucketcloud.Repo, error) { assert.Equal(t, "fork", namespace) @@ -658,7 +659,7 @@ func TestBitbucketCloudSource_GetFork(t *testing.T) { }) t.Run("not forked from parent", func(t *testing.T) { - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) user := &bitbucketcloud.User{ Account: bitbucketcloud.Account{ @@ -695,7 +696,7 @@ func TestBitbucketCloudSource_GetFork(t *testing.T) { }) t.Run("error getting current user", func(t *testing.T) { - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) want := errors.New("error") client.CurrentUserFunc.SetDefaultReturn(nil, want) @@ -707,7 +708,7 @@ func TestBitbucketCloudSource_GetFork(t *testing.T) { }) t.Run("success with default namespace, name", func(t *testing.T) { - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) user := &bitbucketcloud.User{ Account: bitbucketcloud.Account{ @@ -729,7 +730,7 @@ func TestBitbucketCloudSource_GetFork(t *testing.T) { }) t.Run("success with default name", func(t *testing.T) { - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) client.RepoFunc.SetDefaultHook(func(ctx context.Context, namespace, slug string) (*bitbucketcloud.Repo, error) { assert.Equal(t, "fork", namespace) @@ -753,7 +754,7 @@ func TestBitbucketCloudSource_GetFork(t *testing.T) { }) t.Run("success with set namespace, name", func(t *testing.T) { - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) client.RepoFunc.SetDefaultHook(func(ctx context.Context, namespace, slug string) (*bitbucketcloud.Repo, error) { assert.Equal(t, "fork", namespace) @@ -785,7 +786,7 @@ func TestBitbucketCloudSource_annotatePullRequest(t *testing.T) { ctx := context.Background() t.Run("error getting all statuses", func(t *testing.T) { - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) _, _, bbRepo := mockBitbucketCloudChangeset() pr := mockBitbucketCloudPullRequest(bbRepo) @@ -805,7 +806,7 @@ func TestBitbucketCloudSource_annotatePullRequest(t *testing.T) { }) t.Run("success", func(t *testing.T) { - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) _, _, bbRepo := mockBitbucketCloudChangeset() pr := mockBitbucketCloudPullRequest(bbRepo) @@ -845,7 +846,7 @@ func TestBitbucketCloudSource_setChangesetMetadata(t *testing.T) { // happens if Changeset.SetMetadata returns an error, so let's set that up. ctx := context.Background() - s, client := mockBitbucketCloudSource() + s, client := mockBitbucketCloudSource(t) mockAnnotatePullRequestSuccess(client) cs, _, bbRepo := mockBitbucketCloudChangeset() @@ -927,9 +928,9 @@ func annotateBitbucketCloudChangesetWithPullRequest(cs *Changeset, pr *bitbucket } } -func mockBitbucketCloudSource() (*BitbucketCloudSource, *MockBitbucketCloudClient) { +func mockBitbucketCloudSource(t *testing.T) (*BitbucketCloudSource, *MockBitbucketCloudClient) { client := NewStrictMockBitbucketCloudClient() - s := &BitbucketCloudSource{client: client} + s := &BitbucketCloudSource{client: client, logger: logtest.Scoped(t)} return s, client } diff --git a/internal/batches/sources/bitbucketserver.go b/internal/batches/sources/bitbucketserver.go index ef24366e3c9be..883e5896ac812 100644 --- a/internal/batches/sources/bitbucketserver.go +++ b/internal/batches/sources/bitbucketserver.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/inconshreveable/log15" //nolint:logging // TODO move all logging to sourcegraph/log + logger "github.com/sourcegraph/log" btypes "github.com/sourcegraph/sourcegraph/internal/batches/types" @@ -24,12 +25,15 @@ import ( type BitbucketServerSource struct { client *bitbucketserver.Client au auth.Authenticator + logger logger.Logger } -var _ ForkableChangesetSource = BitbucketServerSource{} +var _ ForkableChangesetSource = &BitbucketServerSource{} // NewBitbucketServerSource returns a new BitbucketServerSource from the given external service. -func NewBitbucketServerSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory) (*BitbucketServerSource, error) { +func NewBitbucketServerSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory, logger logger.Logger) (*BitbucketServerSource, error) { + logger = logger.Scoped("BitbucketServerSource") + rawConfig, err := svc.Config.Decrypt(ctx) if err != nil { return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err) @@ -40,7 +44,7 @@ func NewBitbucketServerSource(ctx context.Context, svc *types.ExternalService, c } if cf == nil { - cf = httpcli.ExternalClientFactory + cf = httpcli.ExternalClientFactory(logger) } opts := httpClientCertificateOptions(nil, c.Certificate) @@ -50,7 +54,7 @@ func NewBitbucketServerSource(ctx context.Context, svc *types.ExternalService, c return nil, err } - client, err := bitbucketserver.NewClient(svc.URN(), &c, cli) + client, err := bitbucketserver.NewClient(svc.URN(), &c, cli, logger) if err != nil { return nil, err } @@ -58,18 +62,19 @@ func NewBitbucketServerSource(ctx context.Context, svc *types.ExternalService, c return &BitbucketServerSource{ au: client.Auth, client: client, + logger: logger, }, nil } -func (s BitbucketServerSource) GitserverPushConfig(ctx context.Context, repo *types.Repo) (*protocol.PushConfig, error) { - return GitserverPushConfig(ctx, repo, s.au) +func (s *BitbucketServerSource) GitserverPushConfig(ctx context.Context, repo *types.Repo) (*protocol.PushConfig, error) { + return GitserverPushConfig(ctx, repo, s.au, s.logger) } -func (s BitbucketServerSource) AuthenticationStrategy() AuthenticationStrategy { +func (s *BitbucketServerSource) AuthenticationStrategy() AuthenticationStrategy { return AuthenticationStrategyUserCredential } -func (s BitbucketServerSource) WithAuthenticator(a auth.Authenticator) (ChangesetSource, error) { +func (s *BitbucketServerSource) WithAuthenticator(a auth.Authenticator) (ChangesetSource, error) { switch a.(type) { case *auth.OAuthBearerToken, *auth.OAuthBearerTokenWithSSH, @@ -91,17 +96,17 @@ func (s BitbucketServerSource) WithAuthenticator(a auth.Authenticator) (Changese // AuthenticatedUsername uses the underlying bitbucketserver.Client to get the // username belonging to the credentials associated with the // BitbucketServerSource. -func (s BitbucketServerSource) AuthenticatedUsername(ctx context.Context) (string, error) { +func (s *BitbucketServerSource) AuthenticatedUsername(ctx context.Context) (string, error) { return s.client.AuthenticatedUsername(ctx) } -func (s BitbucketServerSource) ValidateAuthenticator(ctx context.Context) error { +func (s *BitbucketServerSource) ValidateAuthenticator(ctx context.Context) error { _, err := s.client.AuthenticatedUsername(ctx) return err } // CreateChangeset creates the given *Changeset in the code host. -func (s BitbucketServerSource) CreateChangeset(ctx context.Context, c *Changeset) (bool, error) { +func (s *BitbucketServerSource) CreateChangeset(ctx context.Context, c *Changeset) (bool, error) { var exists bool remoteRepo := c.RemoteRepo.Metadata.(*bitbucketserver.Repo) @@ -146,7 +151,7 @@ func (s BitbucketServerSource) CreateChangeset(ctx context.Context, c *Changeset // CloseChangeset closes the given *Changeset on the code host and updates the // Metadata column in the *batches.Changeset to the newly closed pull request. -func (s BitbucketServerSource) CloseChangeset(ctx context.Context, c *Changeset) error { +func (s *BitbucketServerSource) CloseChangeset(ctx context.Context, c *Changeset) error { pr, ok := c.Changeset.Metadata.(*bitbucketserver.PullRequest) if !ok { return errors.New("Changeset is not a Bitbucket Server pull request") @@ -169,7 +174,7 @@ func (s BitbucketServerSource) CloseChangeset(ctx context.Context, c *Changeset) } // LoadChangeset loads the latest state of the given Changeset from the codehost. -func (s BitbucketServerSource) LoadChangeset(ctx context.Context, cs *Changeset) error { +func (s *BitbucketServerSource) LoadChangeset(ctx context.Context, cs *Changeset) error { repo := cs.TargetRepo.Metadata.(*bitbucketserver.Repo) number, err := strconv.Atoi(cs.ExternalID) if err != nil { @@ -200,7 +205,7 @@ func (s BitbucketServerSource) LoadChangeset(ctx context.Context, cs *Changeset) return nil } -func (s BitbucketServerSource) loadPullRequestData(ctx context.Context, pr *bitbucketserver.PullRequest) error { +func (s *BitbucketServerSource) loadPullRequestData(ctx context.Context, pr *bitbucketserver.PullRequest) error { if err := s.client.LoadPullRequestActivities(ctx, pr); err != nil { return errors.Wrap(err, "loading pr activities") } @@ -216,7 +221,7 @@ func (s BitbucketServerSource) loadPullRequestData(ctx context.Context, pr *bitb return nil } -func (s BitbucketServerSource) UpdateChangeset(ctx context.Context, c *Changeset) error { +func (s *BitbucketServerSource) UpdateChangeset(ctx context.Context, c *Changeset) error { pr, ok := c.Changeset.Metadata.(*bitbucketserver.PullRequest) if !ok { return errors.New("Changeset is not a Bitbucket Server pull request") @@ -265,7 +270,7 @@ func (s BitbucketServerSource) UpdateChangeset(ctx context.Context, c *Changeset // ReopenChangeset reopens the *Changeset on the code host and updates the // Metadata column in the *batches.Changeset. -func (s BitbucketServerSource) ReopenChangeset(ctx context.Context, c *Changeset) error { +func (s *BitbucketServerSource) ReopenChangeset(ctx context.Context, c *Changeset) error { reopened, err := s.callAndRetryIfOutdated(ctx, c, s.client.ReopenPullRequest) if err != nil { return err @@ -276,7 +281,7 @@ func (s BitbucketServerSource) ReopenChangeset(ctx context.Context, c *Changeset } // CreateComment posts a comment on the Changeset. -func (s BitbucketServerSource) CreateComment(ctx context.Context, c *Changeset, text string) error { +func (s *BitbucketServerSource) CreateComment(ctx context.Context, c *Changeset, text string) error { // Bitbucket Server seems to ignore version conflicts when commenting, but // we use this here anyway. _, err := s.callAndRetryIfOutdated(ctx, c, func(ctx context.Context, pr *bitbucketserver.PullRequest) error { @@ -288,7 +293,7 @@ func (s BitbucketServerSource) CreateComment(ctx context.Context, c *Changeset, // MergeChangeset merges a Changeset on the code host, if in a mergeable state. // The squash parameter is ignored, as Bitbucket Server does not support // squash merges. -func (s BitbucketServerSource) MergeChangeset(ctx context.Context, c *Changeset, squash bool) error { +func (s *BitbucketServerSource) MergeChangeset(ctx context.Context, c *Changeset, squash bool) error { pr, ok := c.Changeset.Metadata.(*bitbucketserver.PullRequest) if !ok { return errors.New("Changeset is not a Bitbucket Server pull request") @@ -315,7 +320,7 @@ func (s BitbucketServerSource) MergeChangeset(ctx context.Context, c *Changeset, type bitbucketClientFunc func(context.Context, *bitbucketserver.PullRequest) error -func (s BitbucketServerSource) callAndRetryIfOutdated(ctx context.Context, c *Changeset, fn bitbucketClientFunc) (*bitbucketserver.PullRequest, error) { +func (s *BitbucketServerSource) callAndRetryIfOutdated(ctx context.Context, c *Changeset, fn bitbucketClientFunc) (*bitbucketserver.PullRequest, error) { pr, ok := c.Changeset.Metadata.(*bitbucketserver.PullRequest) if !ok { return nil, errors.New("Changeset is not a Bitbucket Server pull request") @@ -352,7 +357,7 @@ func (s BitbucketServerSource) callAndRetryIfOutdated(ctx context.Context, c *Ch // exists and creating it if it doesn't. If namespace is not provided, the fork will be in // the currently authenticated user's namespace. If name is not provided, the fork will be // named with the default Sourcegraph convention: "${original-namespace}-${original-name}" -func (s BitbucketServerSource) GetFork(ctx context.Context, targetRepo *types.Repo, ns, n *string) (*types.Repo, error) { +func (s *BitbucketServerSource) GetFork(ctx context.Context, targetRepo *types.Repo, ns, n *string) (*types.Repo, error) { var namespace string if ns != nil { namespace = *ns @@ -394,11 +399,11 @@ func (s BitbucketServerSource) GetFork(ctx context.Context, targetRepo *types.Re return s.checkAndCopy(targetRepo, fork, namespace) } -func (s BitbucketServerSource) BuildCommitOpts(repo *types.Repo, _ *btypes.Changeset, spec *btypes.ChangesetSpec, pushOpts *protocol.PushConfig) protocol.CreateCommitFromPatchRequest { +func (s *BitbucketServerSource) BuildCommitOpts(repo *types.Repo, _ *btypes.Changeset, spec *btypes.ChangesetSpec, pushOpts *protocol.PushConfig) protocol.CreateCommitFromPatchRequest { return BuildCommitOptsCommon(repo, spec, pushOpts) } -func (s BitbucketServerSource) checkAndCopy(targetRepo *types.Repo, fork *bitbucketserver.Repo, forkNamespace string) (*types.Repo, error) { +func (s *BitbucketServerSource) checkAndCopy(targetRepo *types.Repo, fork *bitbucketserver.Repo, forkNamespace string) (*types.Repo, error) { tr := targetRepo.Metadata.(*bitbucketserver.Repo) if fork.Origin == nil { diff --git a/internal/batches/sources/bitbucketserver_test.go b/internal/batches/sources/bitbucketserver_test.go index 2c2a052fd24f8..34c686776e0e0 100644 --- a/internal/batches/sources/bitbucketserver_test.go +++ b/internal/batches/sources/bitbucketserver_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/inconshreveable/log15" //nolint:logging // TODO move all logging to sourcegraph/log + "github.com/sourcegraph/log/logtest" "github.com/stretchr/testify/assert" btypes "github.com/sourcegraph/sourcegraph/internal/batches/types" @@ -81,7 +82,7 @@ func TestBitbucketServerSource_LoadChangeset(t *testing.T) { } ctx := context.Background() - bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf) + bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -197,7 +198,7 @@ func TestBitbucketServerSource_CreateChangeset(t *testing.T) { } ctx := context.Background() - bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf) + bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -283,7 +284,7 @@ func TestBitbucketServerSource_CloseChangeset(t *testing.T) { } ctx := context.Background() - bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf) + bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -362,7 +363,7 @@ func TestBitbucketServerSource_CloseChangeset_DeleteSourceBranch(t *testing.T) { } ctx := context.Background() - bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf) + bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -442,7 +443,7 @@ func TestBitbucketServerSource_ReopenChangeset(t *testing.T) { } ctx := context.Background() - bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf) + bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -546,7 +547,7 @@ func TestBitbucketServerSource_UpdateChangeset(t *testing.T) { } ctx := context.Background() - bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf) + bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -626,7 +627,7 @@ func TestBitbucketServerSource_CreateComment(t *testing.T) { } ctx := context.Background() - bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf) + bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -711,7 +712,7 @@ func TestBitbucketServerSource_MergeChangeset(t *testing.T) { } ctx := context.Background() - bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf) + bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -749,7 +750,7 @@ func TestBitbucketServerSource_WithAuthenticator(t *testing.T) { } ctx := context.Background() - bbsSrc, err := NewBitbucketServerSource(ctx, svc, nil) + bbsSrc, err := NewBitbucketServerSource(ctx, svc, nil, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -855,7 +856,7 @@ func TestBitbucketServerSource_GetFork(t *testing.T) { svc := newExternalService(t, pointers.Ptr("invalid")) ctx := context.Background() - bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf) + bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf, logtest.Scoped(t)) assert.Nil(t, err) fork, err := bbsSrc.GetFork(ctx, newBitbucketServerRepo(urn, "SOUR", "read-only", 10103), nil, nil) @@ -880,7 +881,7 @@ func TestBitbucketServerSource_GetFork(t *testing.T) { target := newBitbucketServerRepo(urn, "BAT", "vcr-fork-test-repo", 0) ctx := context.Background() - bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf) + bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf, logtest.Scoped(t)) assert.Nil(t, err) fork, err := bbsSrc.GetFork(ctx, target, pointers.Ptr("~milton"), pointers.Ptr("vcr-fork-test-repo")) @@ -906,7 +907,7 @@ func TestBitbucketServerSource_GetFork(t *testing.T) { target := newBitbucketServerRepo(urn, "BAT", "vcr-fork-test-repo-already-forked", 0) ctx := context.Background() - bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf) + bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf, logtest.Scoped(t)) assert.Nil(t, err) fork, err := bbsSrc.GetFork(ctx, target, pointers.Ptr("~milton"), pointers.Ptr("BAT-vcr-fork-test-repo-already-forked")) @@ -935,7 +936,7 @@ func TestBitbucketServerSource_GetFork(t *testing.T) { target := newBitbucketServerRepo(urn, "BAT", "vcr-fork-test-repo-already-forked", 24378) ctx := context.Background() - bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf) + bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf, logtest.Scoped(t)) assert.Nil(t, err) username, err := bbsSrc.client.AuthenticatedUsername(ctx) @@ -976,7 +977,7 @@ func TestBitbucketServerSource_GetFork(t *testing.T) { target := newBitbucketServerRepo(urn, "BAT", "vcr-fork-test-repo-not-forked", 216974) ctx := context.Background() - bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf) + bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf, logtest.Scoped(t)) assert.Nil(t, err) username, err := bbsSrc.client.AuthenticatedUsername(ctx) @@ -1011,7 +1012,7 @@ func TestBitbucketServerSource_GetFork(t *testing.T) { target := newBitbucketServerRepo(urn, "BAT", "vcr-fork-test-repo-already-forked", 24378) ctx := context.Background() - bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf) + bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf, logtest.Scoped(t)) assert.Nil(t, err) username, err := bbsSrc.client.AuthenticatedUsername(ctx) @@ -1052,7 +1053,7 @@ func TestBitbucketServerSource_GetFork(t *testing.T) { target := newBitbucketServerRepo(urn, "BAT", "vcr-fork-test-repo-not-forked", 216974) ctx := context.Background() - bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf) + bbsSrc, err := NewBitbucketServerSource(ctx, svc, cf, logtest.Scoped(t)) assert.Nil(t, err) username, err := bbsSrc.client.AuthenticatedUsername(ctx) diff --git a/internal/batches/sources/gerrit.go b/internal/batches/sources/gerrit.go index dbab3a58a9181..7a00b2ba5d4a0 100644 --- a/internal/batches/sources/gerrit.go +++ b/internal/batches/sources/gerrit.go @@ -9,6 +9,8 @@ import ( "net/url" "strings" + "github.com/sourcegraph/log" + gerritbatches "github.com/sourcegraph/sourcegraph/internal/batches/sources/gerrit" btypes "github.com/sourcegraph/sourcegraph/internal/batches/types" "github.com/sourcegraph/sourcegraph/internal/conf" @@ -26,9 +28,12 @@ import ( type GerritSource struct { client gerrit.Client + logger log.Logger } -func NewGerritSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory) (*GerritSource, error) { +func NewGerritSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory, logger log.Logger) (*GerritSource, error) { + logger = logger.Scoped("GerritSource") + rawConfig, err := svc.Config.Decrypt(ctx) if err != nil { return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err) @@ -39,7 +44,7 @@ func NewGerritSource(ctx context.Context, svc *types.ExternalService, cf *httpcl } if cf == nil { - cf = httpcli.ExternalClientFactory + cf = httpcli.ExternalClientFactory(logger) } cli, err := cf.Doer() @@ -52,28 +57,28 @@ func NewGerritSource(ctx context.Context, svc *types.ExternalService, cf *httpcl return nil, errors.Wrap(err, "parsing Gerrit CodeHostURL") } - client, err := gerrit.NewClient(svc.URN(), gerritURL, &gerrit.AccountCredentials{Username: c.Username, Password: c.Password}, cli) + client, err := gerrit.NewClient(svc.URN(), gerritURL, &gerrit.AccountCredentials{Username: c.Username, Password: c.Password}, cli, logger) if err != nil { return nil, errors.Wrap(err, "creating Gerrit client") } - return &GerritSource{client: client}, nil + return &GerritSource{client: client, logger: logger}, nil } // GitserverPushConfig returns an authenticated push config used for pushing // commits to the code host. -func (s GerritSource) GitserverPushConfig(ctx context.Context, repo *types.Repo) (*protocol.PushConfig, error) { - return GitserverPushConfig(ctx, repo, s.client.Authenticator()) +func (s *GerritSource) GitserverPushConfig(ctx context.Context, repo *types.Repo) (*protocol.PushConfig, error) { + return GitserverPushConfig(ctx, repo, s.client.Authenticator(), s.logger) } -func (s GerritSource) AuthenticationStrategy() AuthenticationStrategy { +func (s *GerritSource) AuthenticationStrategy() AuthenticationStrategy { return AuthenticationStrategyUserCredential } // WithAuthenticator returns a copy of the original Source configured to use the // given authenticator, provided that authenticator type is supported by the // code host. -func (s GerritSource) WithAuthenticator(a auth.Authenticator) (ChangesetSource, error) { +func (s *GerritSource) WithAuthenticator(a auth.Authenticator) (ChangesetSource, error) { client, err := s.client.WithAuthenticator(a) if err != nil { return nil, err @@ -84,7 +89,7 @@ func (s GerritSource) WithAuthenticator(a auth.Authenticator) (ChangesetSource, // ValidateAuthenticator validates the currently set authenticator is usable. // Returns an error, when validating the Authenticator yielded an error. -func (s GerritSource) ValidateAuthenticator(ctx context.Context) error { +func (s *GerritSource) ValidateAuthenticator(ctx context.Context) error { _, err := s.client.GetAuthenticatedUserAccount(ctx) return err } @@ -92,7 +97,7 @@ func (s GerritSource) ValidateAuthenticator(ctx context.Context) error { // LoadChangeset loads the given Changeset from the source and updates it. If // the Changeset could not be found on the source, a ChangesetNotFoundError is // returned. -func (s GerritSource) LoadChangeset(ctx context.Context, cs *Changeset) error { +func (s *GerritSource) LoadChangeset(ctx context.Context, cs *Changeset) error { pr, err := s.client.GetChange(ctx, cs.ExternalID) if err != nil { if errcode.IsNotFound(err) { @@ -105,7 +110,7 @@ func (s GerritSource) LoadChangeset(ctx context.Context, cs *Changeset) error { // CreateChangeset will create the Changeset on the source. If it already // exists, *Changeset will be populated and the return value will be true. -func (s GerritSource) CreateChangeset(ctx context.Context, cs *Changeset) (bool, error) { +func (s *GerritSource) CreateChangeset(ctx context.Context, cs *Changeset) (bool, error) { changeID := GenerateGerritChangeID(*cs.Changeset) // For Gerrit, the Change is created at `git push` time, so we just load it here to verify it // was created successfully. @@ -124,7 +129,7 @@ func (s GerritSource) CreateChangeset(ctx context.Context, cs *Changeset) (bool, } // CreateDraftChangeset creates the given changeset on the code host in draft mode. -func (s GerritSource) CreateDraftChangeset(ctx context.Context, cs *Changeset) (bool, error) { +func (s *GerritSource) CreateDraftChangeset(ctx context.Context, cs *Changeset) (bool, error) { changeID := GenerateGerritChangeID(*cs.Changeset) // For Gerrit, the Change is created at `git push` time, so we just call the API to mark it as WIP. @@ -149,7 +154,7 @@ func (s GerritSource) CreateDraftChangeset(ctx context.Context, cs *Changeset) ( } // UndraftChangeset will update the Changeset on the source to be not in draft mode anymore. -func (s GerritSource) UndraftChangeset(ctx context.Context, cs *Changeset) error { +func (s *GerritSource) UndraftChangeset(ctx context.Context, cs *Changeset) error { if err := s.client.SetReadyForReview(ctx, cs.ExternalID); err != nil { if errcode.IsNotFound(err) { return ChangesetNotFoundError{Changeset: cs} @@ -166,7 +171,7 @@ func (s GerritSource) UndraftChangeset(ctx context.Context, cs *Changeset) error // CloseChangeset will close the Changeset on the source, where "close" // means the appropriate final state on the codehost (e.g. "abandoned" on // Gerrit). -func (s GerritSource) CloseChangeset(ctx context.Context, cs *Changeset) error { +func (s *GerritSource) CloseChangeset(ctx context.Context, cs *Changeset) error { updated, err := s.client.AbandonChange(ctx, cs.ExternalID) if err != nil { return errors.Wrap(err, "abandoning change") @@ -182,7 +187,7 @@ func (s GerritSource) CloseChangeset(ctx context.Context, cs *Changeset) error { } // UpdateChangeset can update Changesets. -func (s GerritSource) UpdateChangeset(ctx context.Context, cs *Changeset) error { +func (s *GerritSource) UpdateChangeset(ctx context.Context, cs *Changeset) error { pr, err := s.client.GetChange(ctx, cs.ExternalID) if err != nil { // Route 1 @@ -235,7 +240,7 @@ func (s GerritSource) UpdateChangeset(ctx context.Context, cs *Changeset) error // ReopenChangeset will reopen the Changeset on the source, if it's closed. // If not, it's a noop. -func (s GerritSource) ReopenChangeset(ctx context.Context, cs *Changeset) error { +func (s *GerritSource) ReopenChangeset(ctx context.Context, cs *Changeset) error { updated, err := s.client.RestoreChange(ctx, cs.ExternalID) if err != nil { return errors.Wrap(err, "restoring change") @@ -245,7 +250,7 @@ func (s GerritSource) ReopenChangeset(ctx context.Context, cs *Changeset) error } // CreateComment posts a comment on the Changeset. -func (s GerritSource) CreateComment(ctx context.Context, cs *Changeset, comment string) error { +func (s *GerritSource) CreateComment(ctx context.Context, cs *Changeset, comment string) error { return s.client.WriteReviewComment(ctx, cs.ExternalID, gerrit.ChangeReviewComment{ Message: comment, }) @@ -257,7 +262,7 @@ func (s GerritSource) CreateComment(ctx context.Context, cs *Changeset, comment // merge. If the changeset cannot be merged, because it is in an unmergeable // state, ChangesetNotMergeableError must be returned. // Gerrit changes are always single commit, so squash does not matter. -func (s GerritSource) MergeChangeset(ctx context.Context, cs *Changeset, _ bool) error { +func (s *GerritSource) MergeChangeset(ctx context.Context, cs *Changeset, _ bool) error { updated, err := s.client.SubmitChange(ctx, cs.ExternalID) if err != nil { if errcode.IsNotFound(err) { @@ -268,7 +273,7 @@ func (s GerritSource) MergeChangeset(ctx context.Context, cs *Changeset, _ bool) return errors.Wrap(s.setChangesetMetadata(ctx, updated, cs), "setting Gerrit changeset metadata") } -func (s GerritSource) BuildCommitOpts(repo *types.Repo, changeset *btypes.Changeset, spec *btypes.ChangesetSpec, pushOpts *protocol.PushConfig) protocol.CreateCommitFromPatchRequest { +func (s *GerritSource) BuildCommitOpts(repo *types.Repo, changeset *btypes.Changeset, spec *btypes.ChangesetSpec, pushOpts *protocol.PushConfig) protocol.CreateCommitFromPatchRequest { opts := BuildCommitOptsCommon(repo, spec, pushOpts) pushRef := strings.Replace(gitdomain.EnsureRefPrefix(spec.BaseRef), "refs/heads", "refs/for", 1) //Magical Gerrit ref for pushing changes. opts.PushRef = &pushRef @@ -283,7 +288,7 @@ func (s GerritSource) BuildCommitOpts(repo *types.Repo, changeset *btypes.Change return opts } -func (s GerritSource) setChangesetMetadata(ctx context.Context, change *gerrit.Change, cs *Changeset) error { +func (s *GerritSource) setChangesetMetadata(ctx context.Context, change *gerrit.Change, cs *Changeset) error { apr, err := s.annotateChange(ctx, change) if err != nil { return errors.Wrap(err, "annotating Change") @@ -294,7 +299,7 @@ func (s GerritSource) setChangesetMetadata(ctx context.Context, change *gerrit.C return nil } -func (s GerritSource) annotateChange(ctx context.Context, change *gerrit.Change) (*gerritbatches.AnnotatedChange, error) { +func (s *GerritSource) annotateChange(ctx context.Context, change *gerrit.Change) (*gerritbatches.AnnotatedChange, error) { reviewers, err := s.client.GetChangeReviews(ctx, change.ChangeID) if err != nil { return nil, err diff --git a/internal/batches/sources/gerrit_test.go b/internal/batches/sources/gerrit_test.go index 0ee18bbb9879b..4899c25586467 100644 --- a/internal/batches/sources/gerrit_test.go +++ b/internal/batches/sources/gerrit_test.go @@ -6,6 +6,7 @@ import ( "net/url" "testing" + "github.com/sourcegraph/log/logtest" "github.com/stretchr/testify/assert" gerritbatches "github.com/sourcegraph/sourcegraph/internal/batches/sources/gerrit" @@ -36,7 +37,7 @@ func TestGerritSource_GitserverPushConfig(t *testing.T) { // // So, cue the boilerplate: au := auth.BasicAuth{Username: "user", Password: "pass"} - s, client := mockGerritSource() + s, client := mockGerritSource(t) client.AuthenticatorFunc.SetDefaultReturn(&au) repo := &types.Repo{ @@ -66,7 +67,7 @@ func TestGerritSource_WithAuthenticator(t *testing.T) { t.Run("supports BasicAuth", func(t *testing.T) { newClient := NewStrictMockGerritClient() au := &auth.BasicAuth{} - s, client := mockGerritSource() + s, client := mockGerritSource(t) client.WithAuthenticatorFunc.SetDefaultHook(func(a auth.Authenticator) (gerrit.Client, error) { assert.Same(t, au, a) return newClient, nil @@ -86,7 +87,7 @@ func TestGerritSource_ValidateAuthenticator(t *testing.T) { "error": errors.New("error"), } { t.Run(name, func(t *testing.T) { - s, client := mockGerritSource() + s, client := mockGerritSource(t) client.GetAuthenticatedUserAccountFunc.SetDefaultReturn(&gerrit.Account{}, want) assert.Equal(t, want, s.ValidateAuthenticator(ctx)) @@ -100,7 +101,7 @@ func TestGerritSource_LoadChangeset(t *testing.T) { t.Run("error getting pull request", func(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) want := errors.New("error") client.GetChangeFunc.SetDefaultHook(func(ctx context.Context, changeID string) (*gerrit.Change, error) { assert.Equal(t, changeID, id) @@ -115,7 +116,7 @@ func TestGerritSource_LoadChangeset(t *testing.T) { t.Run("pull request not found", func(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) client.GetChangeFunc.SetDefaultHook(func(ctx context.Context, changeID string) (*gerrit.Change, error) { assert.Equal(t, changeID, id) return &gerrit.Change{}, ¬FoundError{} @@ -131,7 +132,7 @@ func TestGerritSource_LoadChangeset(t *testing.T) { t.Run("success", func(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) change := mockGerritChange(&testProject, id) client.GetURLFunc.SetDefaultReturn(&url.URL{}) @@ -151,7 +152,7 @@ func TestGerritSource_CreateChangeset(t *testing.T) { t.Run("error getting pull request", func(t *testing.T) { cs, _, _ := mockGerritChangeset() - s, client := mockGerritSource() + s, client := mockGerritSource(t) testChangeID := GenerateGerritChangeID(*cs.Changeset) want := errors.New("error") client.GetChangeFunc.SetDefaultHook(func(ctx context.Context, changeID string) (*gerrit.Change, error) { @@ -167,7 +168,7 @@ func TestGerritSource_CreateChangeset(t *testing.T) { t.Run("change not found", func(t *testing.T) { cs, id, _ := mockGerritChangeset() - s, client := mockGerritSource() + s, client := mockGerritSource(t) client.GetChangeFunc.SetDefaultHook(func(ctx context.Context, changeID string) (*gerrit.Change, error) { assert.Equal(t, changeID, id) return &gerrit.Change{}, ¬FoundError{} @@ -183,7 +184,7 @@ func TestGerritSource_CreateChangeset(t *testing.T) { t.Run("success", func(t *testing.T) { cs, id, _ := mockGerritChangeset() - s, client := mockGerritSource() + s, client := mockGerritSource(t) change := mockGerritChange(&testProject, id) client.GetURLFunc.SetDefaultReturn(&url.URL{}) @@ -204,7 +205,7 @@ func TestGerritSource_CreateDraftChangeset(t *testing.T) { t.Run("error setting WIP", func(t *testing.T) { cs, id, _ := mockGerritChangeset() - s, client := mockGerritSource() + s, client := mockGerritSource(t) want := errors.New("error") client.SetWIPFunc.SetDefaultHook(func(ctx context.Context, changeID string) error { assert.Equal(t, changeID, id) @@ -219,7 +220,7 @@ func TestGerritSource_CreateDraftChangeset(t *testing.T) { t.Run("change not found", func(t *testing.T) { cs, id, _ := mockGerritChangeset() - s, client := mockGerritSource() + s, client := mockGerritSource(t) client.SetWIPFunc.SetDefaultHook(func(ctx context.Context, changeID string) error { assert.Equal(t, changeID, id) return ¬FoundError{} @@ -235,7 +236,7 @@ func TestGerritSource_CreateDraftChangeset(t *testing.T) { t.Run("GetChange error", func(t *testing.T) { cs, id, _ := mockGerritChangeset() - s, client := mockGerritSource() + s, client := mockGerritSource(t) want := errors.New("error") client.GetURLFunc.SetDefaultReturn(&url.URL{}) client.SetWIPFunc.SetDefaultHook(func(ctx context.Context, changeID string) error { @@ -256,7 +257,7 @@ func TestGerritSource_CreateDraftChangeset(t *testing.T) { t.Run("success", func(t *testing.T) { cs, id, _ := mockGerritChangeset() - s, client := mockGerritSource() + s, client := mockGerritSource(t) change := mockGerritChange(&testProject, id) client.GetURLFunc.SetDefaultReturn(&url.URL{}) client.SetWIPFunc.SetDefaultHook(func(ctx context.Context, changeID string) error { @@ -286,7 +287,7 @@ func TestGerritSource_UpdateChangeset(t *testing.T) { ID: testChangeIDPrefix + id, }, } - s, client := mockGerritSource() + s, client := mockGerritSource(t) want := errors.New("error") client.GetChangeFunc.SetDefaultHook(func(ctx context.Context, changeID string) (*gerrit.Change, error) { assert.Equal(t, id, changeID) @@ -305,7 +306,7 @@ func TestGerritSource_UpdateChangeset(t *testing.T) { ID: testChangeIDPrefix + id, }, } - s, client := mockGerritSource() + s, client := mockGerritSource(t) want := errors.New("error") client.GetChangeFunc.SetDefaultHook(func(ctx context.Context, changeID string) (*gerrit.Change, error) { assert.Equal(t, id, changeID) @@ -329,7 +330,7 @@ func TestGerritSource_UpdateChangeset(t *testing.T) { WorkInProgress: true, }, } - s, client := mockGerritSource() + s, client := mockGerritSource(t) want := errors.New("error") client.GetChangeFunc.SetDefaultHook(func(ctx context.Context, changeID string) (*gerrit.Change, error) { assert.Equal(t, id, changeID) @@ -357,7 +358,7 @@ func TestGerritSource_UpdateChangeset(t *testing.T) { WorkInProgress: true, }, } - s, client := mockGerritSource() + s, client := mockGerritSource(t) want := errors.New("error") client.GetChangeFunc.SetDefaultHook(func(ctx context.Context, changeID string) (*gerrit.Change, error) { assert.Equal(t, id, changeID) @@ -390,7 +391,7 @@ func TestGerritSource_UpdateChangeset(t *testing.T) { }, } change := mockGerritChange(&testProject, id) - s, client := mockGerritSource() + s, client := mockGerritSource(t) client.DeleteChangeFunc.SetDefaultHook(func(ctx context.Context, changeID string) error { assert.Equal(t, testChangeIDPrefix+id, changeID) return nil @@ -427,7 +428,7 @@ func TestGerritSource_UpdateChangeset(t *testing.T) { } change := mockGerritChange(&testProject, id) change.Branch = "diffbranch" - s, client := mockGerritSource() + s, client := mockGerritSource(t) want := errors.New("error") client.GetChangeFunc.SetDefaultHook(func(ctx context.Context, changeID string) (*gerrit.Change, error) { assert.Equal(t, id, changeID) @@ -455,7 +456,7 @@ func TestGerritSource_UpdateChangeset(t *testing.T) { ogChange := *change change.Branch = "diffbranch" change.Subject = "diffsubject" - s, client := mockGerritSource() + s, client := mockGerritSource(t) want := errors.New("error") client.GetChangeFunc.SetDefaultHook(func(ctx context.Context, changeID string) (*gerrit.Change, error) { assert.Equal(t, id, changeID) @@ -488,7 +489,7 @@ func TestGerritSource_UpdateChangeset(t *testing.T) { ogChange := *change change.Branch = "diffbranch" change.Subject = "diffsubject" - s, client := mockGerritSource() + s, client := mockGerritSource(t) client.GetChangeFunc.SetDefaultReturn(change, nil) client.GetChangeReviewsFunc.SetDefaultReturn(&[]gerrit.Reviewer{}, nil) client.GetURLFunc.SetDefaultReturn(&url.URL{}) @@ -514,7 +515,7 @@ func TestGerritSource_UndraftChangeset(t *testing.T) { t.Run("error setting ReadyForReview", func(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) want := errors.New("error") client.SetReadyForReviewFunc.SetDefaultHook(func(ctx context.Context, changeID string) error { assert.Equal(t, changeID, id) @@ -529,7 +530,7 @@ func TestGerritSource_UndraftChangeset(t *testing.T) { t.Run("change not found", func(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) client.SetReadyForReviewFunc.SetDefaultHook(func(ctx context.Context, changeID string) error { assert.Equal(t, changeID, id) return ¬FoundError{} @@ -545,7 +546,7 @@ func TestGerritSource_UndraftChangeset(t *testing.T) { t.Run("GetChange error", func(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) want := errors.New("error") client.GetURLFunc.SetDefaultReturn(&url.URL{}) @@ -567,7 +568,7 @@ func TestGerritSource_UndraftChangeset(t *testing.T) { t.Run("success", func(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) change := mockGerritChange(&testProject, id) client.GetURLFunc.SetDefaultReturn(&url.URL{}) @@ -592,7 +593,7 @@ func TestGerritSource_CloseChangeset(t *testing.T) { t.Run("error declining pull request", func(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) want := errors.New("error") client.AbandonChangeFunc.SetDefaultHook(func(ctx context.Context, changeID string) (*gerrit.Change, error) { @@ -608,7 +609,7 @@ func TestGerritSource_CloseChangeset(t *testing.T) { t.Run("success", func(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) pr := mockGerritChange(&testProject, id) client.GetURLFunc.SetDefaultReturn(&url.URL{}) @@ -637,7 +638,7 @@ func TestGerritSource_CloseChangeset(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) want := errors.New("error") client.AbandonChangeFunc.SetDefaultHook(func(ctx context.Context, changeID string) (*gerrit.Change, error) { @@ -664,7 +665,7 @@ func TestGerritSource_CloseChangeset(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) pr := mockGerritChange(&testProject, id) client.GetURLFunc.SetDefaultReturn(&url.URL{}) @@ -694,7 +695,7 @@ func TestGerritSource_CreateComment(t *testing.T) { t.Run("error creating comment", func(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) want := errors.New("error") client.WriteReviewCommentFunc.SetDefaultHook(func(ctx context.Context, changeID string, ci gerrit.ChangeReviewComment) error { @@ -711,7 +712,7 @@ func TestGerritSource_CreateComment(t *testing.T) { t.Run("success", func(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) client.WriteReviewCommentFunc.SetDefaultHook(func(ctx context.Context, changeID string, ci gerrit.ChangeReviewComment) error { assert.Equal(t, changeID, id) @@ -730,7 +731,7 @@ func TestGerritSource_MergeChangeset(t *testing.T) { t.Run("error merging pull request", func(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) want := errors.New("error") client.SubmitChangeFunc.SetDefaultHook(func(ctx context.Context, changeID string) (*gerrit.Change, error) { @@ -748,7 +749,7 @@ func TestGerritSource_MergeChangeset(t *testing.T) { t.Run("change not found", func(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) want := ¬FoundError{} client.SubmitChangeFunc.SetDefaultHook(func(ctx context.Context, changeID string) (*gerrit.Change, error) { @@ -764,7 +765,7 @@ func TestGerritSource_MergeChangeset(t *testing.T) { t.Run("success with squash", func(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) pr := mockGerritChange(&testProject, id) client.GetURLFunc.SetDefaultReturn(&url.URL{}) @@ -783,7 +784,7 @@ func TestGerritSource_MergeChangeset(t *testing.T) { t.Run("success with no squash", func(t *testing.T) { cs, id, _ := mockGerritChangeset() cs.ExternalID = id - s, client := mockGerritSource() + s, client := mockGerritSource(t) pr := mockGerritChange(&testProject, id) client.GetURLFunc.SetDefaultReturn(&url.URL{}) @@ -835,9 +836,9 @@ func mockGerritChange(project *gerrit.Project, id string) *gerrit.Change { } } -func mockGerritSource() (*GerritSource, *MockGerritClient) { +func mockGerritSource(t *testing.T) (*GerritSource, *MockGerritClient) { client := NewStrictMockGerritClient() - s := &GerritSource{client: client} + s := &GerritSource{client: client, logger: logtest.Scoped(t)} return s, client } diff --git a/internal/batches/sources/github.go b/internal/batches/sources/github.go index 214d55548c01b..e917519646273 100644 --- a/internal/batches/sources/github.go +++ b/internal/batches/sources/github.go @@ -7,6 +7,8 @@ import ( "strings" "time" + "github.com/sourcegraph/log" + btypes "github.com/sourcegraph/sourcegraph/internal/batches/types" "github.com/sourcegraph/sourcegraph/internal/conf" "github.com/sourcegraph/sourcegraph/internal/database" @@ -28,11 +30,12 @@ import ( type GitHubSource struct { client *github.V4Client au auth.Authenticator + logger log.Logger } -var _ ForkableChangesetSource = GitHubSource{} +var _ ForkableChangesetSource = &GitHubSource{} -func NewGitHubSource(ctx context.Context, db database.DB, svc *types.ExternalService, cf *httpcli.Factory) (*GitHubSource, error) { +func NewGitHubSource(ctx context.Context, db database.DB, svc *types.ExternalService, cf *httpcli.Factory, logger log.Logger) (*GitHubSource, error) { rawConfig, err := svc.Config.Decrypt(ctx) if err != nil { return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err) @@ -41,10 +44,12 @@ func NewGitHubSource(ctx context.Context, db database.DB, svc *types.ExternalSer if err := jsonc.Unmarshal(rawConfig, &c); err != nil { return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err) } - return newGitHubSource(ctx, db, svc.URN(), &c, cf) + return newGitHubSource(ctx, db, svc.URN(), &c, cf, logger) } -func newGitHubSource(ctx context.Context, db database.DB, urn string, c *schema.GitHubConnection, cf *httpcli.Factory) (*GitHubSource, error) { +func newGitHubSource(ctx context.Context, db database.DB, urn string, c *schema.GitHubConnection, cf *httpcli.Factory, logger log.Logger) (*GitHubSource, error) { + logger = logger.Scoped("GitHubSource") + baseURL, err := url.Parse(c.Url) if err != nil { return nil, err @@ -54,7 +59,7 @@ func newGitHubSource(ctx context.Context, db database.DB, urn string, c *schema. apiURL, _ := github.APIRoot(baseURL) if cf == nil { - cf = httpcli.ExternalClientFactory + cf = httpcli.ExternalClientFactory(logger) } opts := httpClientCertificateOptions([]httpcli.Opt{ @@ -75,11 +80,12 @@ func newGitHubSource(ctx context.Context, db database.DB, urn string, c *schema. return &GitHubSource{ au: auther, - client: github.NewV4Client(urn, apiURL, auther, cli), + client: github.NewV4Client(urn, apiURL, auther, cli, logger), + logger: logger, }, nil } -func (s GitHubSource) AuthenticationStrategy() AuthenticationStrategy { +func (s *GitHubSource) AuthenticationStrategy() AuthenticationStrategy { // If the authenticator isn't set, we default to user credentials. if s.au == nil { return AuthenticationStrategyUserCredential @@ -93,19 +99,23 @@ func (s GitHubSource) AuthenticationStrategy() AuthenticationStrategy { return AuthenticationStrategyUserCredential } -func (s GitHubSource) GitserverPushConfig(ctx context.Context, repo *types.Repo) (*protocol.PushConfig, error) { - return GitserverPushConfig(ctx, repo, s.au) +func (s *GitHubSource) GitserverPushConfig(ctx context.Context, repo *types.Repo) (*protocol.PushConfig, error) { + return GitserverPushConfig(ctx, repo, s.au, s.logger) } -func (s GitHubSource) WithAuthenticator(a auth.Authenticator) (ChangesetSource, error) { - sc := s +func (s *GitHubSource) WithAuthenticator(a auth.Authenticator) (ChangesetSource, error) { + sc := GitHubSource{} + if s != nil { + sc = *s + } + sc.au = a sc.client = sc.client.WithAuthenticator(a) return &sc, nil } -func (s GitHubSource) ValidateAuthenticator(ctx context.Context) error { +func (s *GitHubSource) ValidateAuthenticator(ctx context.Context) error { _, err := s.client.GetAuthenticatedUser(ctx) return err } @@ -135,7 +145,7 @@ func (s GitHubSource) ValidateAuthenticator(ctx context.Context) error { // to sign commits with the GitHub App authenticating on behalf of the user, rather than // authenticating as the installation. See here for more details: // https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/about-authentication-with-a-github-app -func (s GitHubSource) DuplicateCommit(ctx context.Context, opts protocol.CreateCommitFromPatchRequest, repo *types.Repo, rev string) (*github.RestCommit, error) { +func (s *GitHubSource) DuplicateCommit(ctx context.Context, opts protocol.CreateCommitFromPatchRequest, repo *types.Repo, rev string) (*github.RestCommit, error) { message := strings.Join(opts.CommitInfo.Messages, "\n") repoMetadata := repo.Metadata.(*github.Repository) owner, repoName, err := github.SplitRepositoryNameWithOwner(repoMetadata.NameWithOwner) @@ -174,7 +184,7 @@ func (s GitHubSource) DuplicateCommit(ctx context.Context, opts protocol.CreateC } // CreateChangeset creates the given changeset on the code host. -func (s GitHubSource) CreateChangeset(ctx context.Context, c *Changeset) (bool, error) { +func (s *GitHubSource) CreateChangeset(ctx context.Context, c *Changeset) (bool, error) { input, err := buildCreatePullRequestInput(c) if err != nil { return false, err @@ -184,7 +194,7 @@ func (s GitHubSource) CreateChangeset(ctx context.Context, c *Changeset) (bool, } // CreateDraftChangeset creates the given changeset on the code host in draft mode. -func (s GitHubSource) CreateDraftChangeset(ctx context.Context, c *Changeset) (bool, error) { +func (s *GitHubSource) CreateDraftChangeset(ctx context.Context, c *Changeset) (bool, error) { input, err := buildCreatePullRequestInput(c) if err != nil { return false, err @@ -214,7 +224,7 @@ func buildCreatePullRequestInput(c *Changeset) (*github.CreatePullRequestInput, }, nil } -func (s GitHubSource) createChangeset(ctx context.Context, c *Changeset, prInput *github.CreatePullRequestInput) (bool, error) { +func (s *GitHubSource) createChangeset(ctx context.Context, c *Changeset, prInput *github.CreatePullRequestInput) (bool, error) { var exists bool pr, err := s.client.CreatePullRequest(ctx, prInput) if err != nil { @@ -247,7 +257,7 @@ func (s GitHubSource) createChangeset(ctx context.Context, c *Changeset, prInput // CloseChangeset closes the given *Changeset on the code host and updates the // Metadata column in the *batches.Changeset to the newly closed pull request. -func (s GitHubSource) CloseChangeset(ctx context.Context, c *Changeset) error { +func (s *GitHubSource) CloseChangeset(ctx context.Context, c *Changeset) error { pr, ok := c.Changeset.Metadata.(*github.PullRequest) if !ok { return errors.New("Changeset is not a GitHub pull request") @@ -273,7 +283,7 @@ func (s GitHubSource) CloseChangeset(ctx context.Context, c *Changeset) error { } // UndraftChangeset will update the Changeset on the source to be not in draft mode anymore. -func (s GitHubSource) UndraftChangeset(ctx context.Context, c *Changeset) error { +func (s *GitHubSource) UndraftChangeset(ctx context.Context, c *Changeset) error { pr, ok := c.Changeset.Metadata.(*github.PullRequest) if !ok { return errors.New("Changeset is not a GitHub pull request") @@ -288,7 +298,7 @@ func (s GitHubSource) UndraftChangeset(ctx context.Context, c *Changeset) error } // LoadChangeset loads the latest state of the given Changeset from the codehost. -func (s GitHubSource) LoadChangeset(ctx context.Context, cs *Changeset) error { +func (s *GitHubSource) LoadChangeset(ctx context.Context, cs *Changeset) error { repo := cs.TargetRepo.Metadata.(*github.Repository) number, err := strconv.ParseInt(cs.ExternalID, 10, 64) if err != nil { @@ -315,7 +325,7 @@ func (s GitHubSource) LoadChangeset(ctx context.Context, cs *Changeset) error { } // UpdateChangeset updates the given *Changeset in the code host. -func (s GitHubSource) UpdateChangeset(ctx context.Context, c *Changeset) error { +func (s *GitHubSource) UpdateChangeset(ctx context.Context, c *Changeset) error { pr, ok := c.Changeset.Metadata.(*github.PullRequest) if !ok { return errors.New("Changeset is not a GitHub pull request") @@ -335,7 +345,7 @@ func (s GitHubSource) UpdateChangeset(ctx context.Context, c *Changeset) error { } // ReopenChangeset reopens the given *Changeset on the code host. -func (s GitHubSource) ReopenChangeset(ctx context.Context, c *Changeset) error { +func (s *GitHubSource) ReopenChangeset(ctx context.Context, c *Changeset) error { pr, ok := c.Changeset.Metadata.(*github.PullRequest) if !ok { return errors.New("Changeset is not a GitHub pull request") @@ -350,7 +360,7 @@ func (s GitHubSource) ReopenChangeset(ctx context.Context, c *Changeset) error { } // CreateComment posts a comment on the Changeset. -func (s GitHubSource) CreateComment(ctx context.Context, c *Changeset, text string) error { +func (s *GitHubSource) CreateComment(ctx context.Context, c *Changeset, text string) error { pr, ok := c.Changeset.Metadata.(*github.PullRequest) if !ok { return errors.New("Changeset is not a GitHub pull request") @@ -361,7 +371,7 @@ func (s GitHubSource) CreateComment(ctx context.Context, c *Changeset, text stri // MergeChangeset merges a Changeset on the code host, if in a mergeable state. // If squash is true, a squash-then-merge merge will be performed. -func (s GitHubSource) MergeChangeset(ctx context.Context, c *Changeset, squash bool) error { +func (s *GitHubSource) MergeChangeset(ctx context.Context, c *Changeset, squash bool) error { pr, ok := c.Changeset.Metadata.(*github.PullRequest) if !ok { return errors.New("Changeset is not a GitHub pull request") @@ -392,11 +402,11 @@ func (GitHubSource) IsPushResponseArchived(s string) bool { return strings.Contains(s, "This repository was archived so it is read-only.") } -func (s GitHubSource) GetFork(ctx context.Context, targetRepo *types.Repo, namespace, n *string) (*types.Repo, error) { +func (s *GitHubSource) GetFork(ctx context.Context, targetRepo *types.Repo, namespace, n *string) (*types.Repo, error) { return getGitHubForkInternal(ctx, targetRepo, s.client, namespace, n) } -func (s GitHubSource) BuildCommitOpts(repo *types.Repo, _ *btypes.Changeset, spec *btypes.ChangesetSpec, pushOpts *protocol.PushConfig) protocol.CreateCommitFromPatchRequest { +func (s *GitHubSource) BuildCommitOpts(repo *types.Repo, _ *btypes.Changeset, spec *btypes.ChangesetSpec, pushOpts *protocol.PushConfig) protocol.CreateCommitFromPatchRequest { return BuildCommitOptsCommon(repo, spec, pushOpts) } diff --git a/internal/batches/sources/github_test.go b/internal/batches/sources/github_test.go index 5b1d4d7d794bf..ec25461c65e34 100644 --- a/internal/batches/sources/github_test.go +++ b/internal/batches/sources/github_test.go @@ -11,6 +11,7 @@ import ( "strings" "testing" + "github.com/sourcegraph/log/logtest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -148,7 +149,7 @@ func TestGithubSource_CreateChangeset_CreationLimit(t *testing.T) { apiURL, err := url.Parse("https://fake.api.github.com") require.NoError(t, err) - client := github.NewV4Client("extsvc:github:0", apiURL, nil, cli) + client := github.NewV4Client("extsvc:github:0", apiURL, nil, cli, logtest.Scoped(t)) source := &GitHubSource{ client: client, } @@ -493,7 +494,7 @@ func TestGithubSource_WithAuthenticator(t *testing.T) { } ctx := context.Background() - githubSrc, err := NewGitHubSource(ctx, dbmocks.NewMockDB(), svc, nil) + githubSrc, err := NewGitHubSource(ctx, dbmocks.NewMockDB(), svc, nil, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -898,7 +899,7 @@ func setup(t *testing.T, ctx context.Context, tName string) (src *GitHubSource, })), } - src, err := NewGitHubSource(ctx, dbmocks.NewMockDB(), svc, cf) + src, err := NewGitHubSource(ctx, dbmocks.NewMockDB(), svc, cf, logtest.Scoped(t)) if err != nil { t.Fatal(err) } diff --git a/internal/batches/sources/gitlab.go b/internal/batches/sources/gitlab.go index 350e656038708..601880d36bc6d 100644 --- a/internal/batches/sources/gitlab.go +++ b/internal/batches/sources/gitlab.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/Masterminds/semver" + "github.com/sourcegraph/log" btypes "github.com/sourcegraph/sourcegraph/internal/batches/types" @@ -27,6 +28,7 @@ import ( type GitLabSource struct { client *gitlab.Client au auth.Authenticator + logger log.Logger } var _ ChangesetSource = &GitLabSource{} @@ -34,7 +36,7 @@ var _ DraftChangesetSource = &GitLabSource{} var _ ForkableChangesetSource = &GitLabSource{} // NewGitLabSource returns a new GitLabSource from the given external service. -func NewGitLabSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory) (*GitLabSource, error) { +func NewGitLabSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory, logger log.Logger) (*GitLabSource, error) { rawConfig, err := svc.Config.Decrypt(ctx) if err != nil { return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err) @@ -43,10 +45,12 @@ func NewGitLabSource(ctx context.Context, svc *types.ExternalService, cf *httpcl if err := jsonc.Unmarshal(rawConfig, &c); err != nil { return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err) } - return newGitLabSource(svc.URN(), &c, cf) + return newGitLabSource(svc.URN(), &c, cf, logger) } -func newGitLabSource(urn string, c *schema.GitLabConnection, cf *httpcli.Factory) (*GitLabSource, error) { +func newGitLabSource(urn string, c *schema.GitLabConnection, cf *httpcli.Factory, logger log.Logger) (*GitLabSource, error) { + logger = logger.Scoped("GitLabSource") + baseURL, err := url.Parse(c.Url) if err != nil { return nil, err @@ -54,7 +58,7 @@ func newGitLabSource(urn string, c *schema.GitLabConnection, cf *httpcli.Factory baseURL = extsvc.NormalizeBaseURL(baseURL) if cf == nil { - cf = httpcli.ExternalClientFactory + cf = httpcli.ExternalClientFactory(logger) } opts := httpClientCertificateOptions(nil, c.Certificate) @@ -75,22 +79,23 @@ func newGitLabSource(urn string, c *schema.GitLabConnection, cf *httpcli.Factory } } - provider := gitlab.NewClientProvider(urn, baseURL, cli) + provider := gitlab.NewClientProvider(urn, baseURL, cli, logger) return &GitLabSource{ au: authr, client: provider.GetAuthenticatorClient(authr), + logger: logger, }, nil } -func (s GitLabSource) GitserverPushConfig(ctx context.Context, repo *types.Repo) (*protocol.PushConfig, error) { - return GitserverPushConfig(ctx, repo, s.au) +func (s *GitLabSource) GitserverPushConfig(ctx context.Context, repo *types.Repo) (*protocol.PushConfig, error) { + return GitserverPushConfig(ctx, repo, s.au, s.logger) } -func (s GitLabSource) AuthenticationStrategy() AuthenticationStrategy { +func (s *GitLabSource) AuthenticationStrategy() AuthenticationStrategy { return AuthenticationStrategyUserCredential } -func (s GitLabSource) WithAuthenticator(a auth.Authenticator) (ChangesetSource, error) { +func (s *GitLabSource) WithAuthenticator(a auth.Authenticator) (ChangesetSource, error) { switch a.(type) { case *auth.OAuthBearerToken, *auth.OAuthBearerTokenWithSSH: @@ -100,14 +105,18 @@ func (s GitLabSource) WithAuthenticator(a auth.Authenticator) (ChangesetSource, return nil, newUnsupportedAuthenticatorError("GitLabSource", a) } - sc := s + sc := GitLabSource{} + if s != nil { + sc = *s + } + sc.au = a sc.client = sc.client.WithAuthenticator(a) return &sc, nil } -func (s GitLabSource) ValidateAuthenticator(ctx context.Context) error { +func (s *GitLabSource) ValidateAuthenticator(ctx context.Context) error { return s.client.ValidateToken(ctx) } @@ -572,11 +581,11 @@ func (*GitLabSource) IsPushResponseArchived(s string) bool { return strings.Contains(s, "ERROR: You are not allowed to push code to this project") } -func (s GitLabSource) GetFork(ctx context.Context, targetRepo *types.Repo, namespace, n *string) (*types.Repo, error) { +func (s *GitLabSource) GetFork(ctx context.Context, targetRepo *types.Repo, namespace, n *string) (*types.Repo, error) { return getGitLabForkInternal(ctx, targetRepo, s.client, namespace, n) } -func (s GitLabSource) BuildCommitOpts(repo *types.Repo, _ *btypes.Changeset, spec *btypes.ChangesetSpec, pushOpts *protocol.PushConfig) protocol.CreateCommitFromPatchRequest { +func (s *GitLabSource) BuildCommitOpts(repo *types.Repo, _ *btypes.Changeset, spec *btypes.ChangesetSpec, pushOpts *protocol.PushConfig) protocol.CreateCommitFromPatchRequest { return BuildCommitOptsCommon(repo, spec, pushOpts) } diff --git a/internal/batches/sources/gitlab_test.go b/internal/batches/sources/gitlab_test.go index 277d47c1fafa0..015a27bfa93e6 100644 --- a/internal/batches/sources/gitlab_test.go +++ b/internal/batches/sources/gitlab_test.go @@ -12,6 +12,7 @@ import ( "github.com/Masterminds/semver" "github.com/google/go-cmp/cmp" "github.com/inconshreveable/log15" //nolint:logging // TODO move all logging to sourcegraph/log + "github.com/sourcegraph/log/logtest" "github.com/stretchr/testify/assert" btypes "github.com/sourcegraph/sourcegraph/internal/batches/types" @@ -274,7 +275,7 @@ func TestGitLabSource_ChangesetSource(t *testing.T) { } ctx := context.Background() - gitlabSource, err := NewGitLabSource(ctx, svc, cf) + gitlabSource, err := NewGitLabSource(ctx, svc, cf, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -594,7 +595,7 @@ func TestGitLabSource_ChangesetSource(t *testing.T) { } ctx := context.Background() - gitlabSource, err := NewGitLabSource(ctx, svc, cf) + gitlabSource, err := NewGitLabSource(ctx, svc, cf, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -941,7 +942,7 @@ func TestGitLabSource_ChangesetSource(t *testing.T) { } ctx := context.Background() - gitlabSource, err := NewGitLabSource(ctx, svc, cf) + gitlabSource, err := NewGitLabSource(ctx, svc, cf, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -1076,7 +1077,7 @@ type gitLabChangesetSourceTestProvider struct { // objects, along with a handful of methods to mock underlying // internal/extsvc/gitlab functions. func newGitLabChangesetSourceTestProvider(t *testing.T) *gitLabChangesetSourceTestProvider { - prov := gitlab.NewClientProvider("Test", &url.URL{}, &panicDoer{}) + prov := gitlab.NewClientProvider("Test", &url.URL{}, &panicDoer{}, logtest.Scoped(t)) repo := &types.Repo{Metadata: &gitlab.Project{}} p := &gitLabChangesetSourceTestProvider{ changeset: &Changeset{ @@ -1343,7 +1344,7 @@ func paginatedPipelineIterator(pipelines []*gitlab.Pipeline, pageSize int) func( func TestGitLabSource_WithAuthenticator(t *testing.T) { t.Run("supported", func(t *testing.T) { var src ChangesetSource - src, err := newGitLabSource("Test", &schema.GitLabConnection{}, nil) + src, err := newGitLabSource("Test", &schema.GitLabConnection{}, nil, logtest.Scoped(t)) if err != nil { t.Errorf("unexpected non-nil error: %v", err) } @@ -1367,7 +1368,7 @@ func TestGitLabSource_WithAuthenticator(t *testing.T) { } { t.Run(name, func(t *testing.T) { var src ChangesetSource - src, err := newGitLabSource("Test", &schema.GitLabConnection{}, nil) + src, err := newGitLabSource("Test", &schema.GitLabConnection{}, nil, logtest.Scoped(t)) if err != nil { t.Errorf("unexpected non-nil error: %v", err) } @@ -1561,7 +1562,7 @@ func TestDecorateMergeRequestData(t *testing.T) { Url: "https://gitlab.com", Token: os.Getenv("GITLAB_TOKEN"), }, - cf, + cf, logtest.Scoped(t), ) assert.Nil(t, err) diff --git a/internal/batches/sources/sources.go b/internal/batches/sources/sources.go index 115181ce18f23..fb348fa3fcc13 100644 --- a/internal/batches/sources/sources.go +++ b/internal/batches/sources/sources.go @@ -133,7 +133,7 @@ func NewSourcer(cf *httpcli.Factory) Sourcer { return newSourcer(cf, loadBatchesSource) } -type changesetSourceFactory func(ctx context.Context, tx SourcerStore, cf *httpcli.Factory, extSvc *types.ExternalService) (ChangesetSource, error) +type changesetSourceFactory func(ctx context.Context, tx SourcerStore, cf *httpcli.Factory, extSvc *types.ExternalService, l log.Logger) (ChangesetSource, error) type sourcer struct { logger log.Logger @@ -167,7 +167,7 @@ func (s *sourcer) ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes. return nil, ErrExternalServiceNotGitHub } - css, err := s.newSource(ctx, tx, s.cf, extSvc) + css, err := s.newSource(ctx, tx, s.cf, extSvc, s.logger) if err != nil { return nil, err } @@ -269,7 +269,7 @@ func (s *sourcer) ForUser(ctx context.Context, tx SourcerStore, uid int32, repo if err != nil { return nil, errors.Wrap(err, "loading external service") } - css, err := s.newSource(ctx, tx, s.cf, extSvc) + css, err := s.newSource(ctx, tx, s.cf, extSvc, s.logger) if err != nil { return nil, err } @@ -297,7 +297,7 @@ func (s *sourcer) ForExternalService(ctx context.Context, tx SourcerStore, au au return nil, errors.Wrap(err, "loading external service") } - css, err := s.newSource(ctx, tx, s.cf, extSvc) + css, err := s.newSource(ctx, tx, s.cf, extSvc, s.logger) if err != nil { return nil, err } @@ -308,8 +308,8 @@ func (s *sourcer) ForExternalService(ctx context.Context, tx SourcerStore, au au return css.WithAuthenticator(au) } -func loadBatchesSource(ctx context.Context, tx SourcerStore, cf *httpcli.Factory, extSvc *types.ExternalService) (ChangesetSource, error) { - css, err := buildChangesetSource(ctx, tx, cf, extSvc) +func loadBatchesSource(ctx context.Context, tx SourcerStore, cf *httpcli.Factory, extSvc *types.ExternalService, logger log.Logger) (ChangesetSource, error) { + css, err := buildChangesetSource(ctx, logger, tx, cf, extSvc) if err != nil { return nil, errors.Wrap(err, "building changeset source") } @@ -319,7 +319,9 @@ func loadBatchesSource(ctx context.Context, tx SourcerStore, cf *httpcli.Factory // GitserverPushConfig creates a push configuration given a repo and an // authenticator. This function is only public for testing purposes, and should // not be used otherwise. -func GitserverPushConfig(ctx context.Context, repo *types.Repo, au auth.Authenticator) (*protocol.PushConfig, error) { +func GitserverPushConfig(ctx context.Context, repo *types.Repo, au auth.Authenticator, logger log.Logger) (*protocol.PushConfig, error) { + logger = logger.Scoped("GitserverPushConfig") + // Empty authenticators are not allowed. if au == nil { return nil, ErrNoPushCredentials{} @@ -365,7 +367,7 @@ func GitserverPushConfig(ctx context.Context, repo *types.Repo, au auth.Authenti } case *ghauth.InstallationAuthenticator: if av.NeedsRefresh() { - if err := av.Refresh(ctx, httpcli.ExternalClient); err != nil { + if err := av.Refresh(ctx, httpcli.ExternalClient(logger.Scoped("GithubAuth.InstallationAuthenticator"))); err != nil { return nil, err } } @@ -532,20 +534,22 @@ func loadExternalService(ctx context.Context, s database.ExternalServiceStore, o // buildChangesetSource builds a ChangesetSource for the given repo to load the // changeset state from. -func buildChangesetSource(ctx context.Context, tx SourcerStore, cf *httpcli.Factory, externalService *types.ExternalService) (ChangesetSource, error) { +func buildChangesetSource(ctx context.Context, logger log.Logger, tx SourcerStore, cf *httpcli.Factory, externalService *types.ExternalService) (ChangesetSource, error) { + logger = logger.Scoped("batches") + switch externalService.Kind { case extsvc.KindGitHub: - return NewGitHubSource(ctx, tx.DatabaseDB(), externalService, cf) + return NewGitHubSource(ctx, tx.DatabaseDB(), externalService, cf, logger.Scoped("GithubSource")) case extsvc.KindGitLab: - return NewGitLabSource(ctx, externalService, cf) + return NewGitLabSource(ctx, externalService, cf, logger.Scoped("GitLabSource")) case extsvc.KindBitbucketServer: - return NewBitbucketServerSource(ctx, externalService, cf) + return NewBitbucketServerSource(ctx, externalService, cf, logger.Scoped("BitbucketServerSource")) case extsvc.KindBitbucketCloud: - return NewBitbucketCloudSource(ctx, externalService, cf) + return NewBitbucketCloudSource(ctx, externalService, cf, logger.Scoped("BitbucketCloudSource")) case extsvc.KindAzureDevOps: - return NewAzureDevOpsSource(ctx, externalService, cf) + return NewAzureDevOpsSource(ctx, externalService, cf, logger.Scoped("AzureDevOpsSource")) case extsvc.KindGerrit: - return NewGerritSource(ctx, externalService, cf) + return NewGerritSource(ctx, externalService, cf, logger.Scoped("GerritSource")) case extsvc.KindPerforce: return NewPerforceSource(ctx, gitserver.NewClient("batches.perforcesource"), externalService, cf) default: diff --git a/internal/batches/sources/sources_test.go b/internal/batches/sources/sources_test.go index 990d9f94f3b89..429c8b505a6b1 100644 --- a/internal/batches/sources/sources_test.go +++ b/internal/batches/sources/sources_test.go @@ -8,6 +8,8 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/sourcegraph/log" + "github.com/sourcegraph/log/logtest" "github.com/stretchr/testify/assert" mockassert "github.com/derision-test/go-mockgen/v2/testutil/assert" @@ -324,7 +326,7 @@ func TestGitserverPushConfig(t *testing.T) { }, } - havePushConfig, haveErr := GitserverPushConfig(ctx, repo, tt.authenticator) + havePushConfig, haveErr := GitserverPushConfig(ctx, repo, tt.authenticator, logtest.Scoped(t)) if haveErr != tt.wantErr { t.Fatalf("invalid error returned, want=%v have=%v", tt.wantErr, haveErr) } @@ -755,7 +757,7 @@ func TestGetRemoteRepo(t *testing.T) { } func newMockSourcer(css ChangesetSource) Sourcer { - return newSourcer(nil, func(ctx context.Context, tx SourcerStore, cf *httpcli.Factory, extSvc *types.ExternalService) (ChangesetSource, error) { + return newSourcer(nil, func(ctx context.Context, tx SourcerStore, cf *httpcli.Factory, extSvc *types.ExternalService, l log.Logger) (ChangesetSource, error) { return css, nil }) } diff --git a/internal/batches/sources/testing/BUILD.bazel b/internal/batches/sources/testing/BUILD.bazel index 2ed1d705a6bb9..78284d13b6465 100644 --- a/internal/batches/sources/testing/BUILD.bazel +++ b/internal/batches/sources/testing/BUILD.bazel @@ -13,5 +13,6 @@ go_library( "//internal/gitserver/protocol", "//internal/types", "//lib/errors", + "@com_github_sourcegraph_log//:log", ], ) diff --git a/internal/batches/sources/testing/fake.go b/internal/batches/sources/testing/fake.go index 803ecd6e0e50d..a47529e0e4c51 100644 --- a/internal/batches/sources/testing/fake.go +++ b/internal/batches/sources/testing/fake.go @@ -3,6 +3,8 @@ package testing import ( "context" + "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/internal/batches/sources" btypes "github.com/sourcegraph/sourcegraph/internal/batches/types" "github.com/sourcegraph/sourcegraph/internal/extsvc/auth" @@ -293,7 +295,8 @@ func (s *FakeChangesetSource) CreateComment(ctx context.Context, c *sources.Chan } func (s *FakeChangesetSource) GitserverPushConfig(ctx context.Context, repo *types.Repo) (*protocol.PushConfig, error) { - return sources.GitserverPushConfig(ctx, repo, s.CurrentAuthenticator) + logger := log.NoOp() // Note: This is fine since this is only used in tests. + return sources.GitserverPushConfig(ctx, repo, s.CurrentAuthenticator, logger) } func (s *FakeChangesetSource) WithAuthenticator(a auth.Authenticator) (sources.ChangesetSource, error) { diff --git a/internal/codemonitors/background/slack.go b/internal/codemonitors/background/slack.go index cc0de0a2dc413..9468050e37fde 100644 --- a/internal/codemonitors/background/slack.go +++ b/internal/codemonitors/background/slack.go @@ -10,14 +10,16 @@ import ( "strings" "github.com/slack-go/slack" + "github.com/sourcegraph/log" "github.com/sourcegraph/sourcegraph/internal/httpcli" searchresult "github.com/sourcegraph/sourcegraph/internal/search/result" "github.com/sourcegraph/sourcegraph/lib/errors" ) -func sendSlackNotification(ctx context.Context, url string, args actionArgs) error { - return postSlackWebhook(ctx, httpcli.ExternalDoer, url, slackPayload(args)) +func sendSlackNotification(ctx context.Context, logger log.Logger, url string, args actionArgs) error { + logger = logger.Scoped("sendSlackNotification") + return postSlackWebhook(ctx, httpcli.ExternalDoer(logger), url, slackPayload(args)) } func slackPayload(args actionArgs) *slack.WebhookMessage { diff --git a/internal/codemonitors/background/webhook.go b/internal/codemonitors/background/webhook.go index d5956597864f5..e85797f287a3f 100644 --- a/internal/codemonitors/background/webhook.go +++ b/internal/codemonitors/background/webhook.go @@ -8,13 +8,15 @@ import ( "net/http" "net/url" + "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/internal/httpcli" "github.com/sourcegraph/sourcegraph/internal/search/result" "github.com/sourcegraph/sourcegraph/lib/errors" ) -func sendWebhookNotification(ctx context.Context, url string, args actionArgs) error { - return postWebhook(ctx, httpcli.ExternalDoer, url, generateWebhookPayload(args)) +func sendWebhookNotification(ctx context.Context, url string, args actionArgs, logger log.Logger) error { + return postWebhook(ctx, httpcli.ExternalDoer(logger), url, generateWebhookPayload(args)) } func postWebhook(ctx context.Context, doer httpcli.Doer, url string, payload webhookPayload) error { diff --git a/internal/codemonitors/background/workers.go b/internal/codemonitors/background/workers.go index f1622f38d84b4..f79492d9ebf03 100644 --- a/internal/codemonitors/background/workers.go +++ b/internal/codemonitors/background/workers.go @@ -103,7 +103,7 @@ func newActionRunner(ctx context.Context, observationCtx *observation.Context, s store := createDBWorkerStoreForActionJobs(observationCtx, s) - worker := dbworker.NewWorker(ctx, store, &actionRunner{s}, options) + worker := dbworker.NewWorker(ctx, store, &actionRunner{CodeMonitorStore: s, logger: observationCtx.Logger.Scoped("CodeMonitorsActionJobsRunner")}, options) return worker } @@ -212,6 +212,7 @@ func (r *queryRunner) Handle(ctx context.Context, logger log.Logger, triggerJob type actionRunner struct { database.CodeMonitorStore + logger log.Logger } func (r *actionRunner) Handle(ctx context.Context, logger log.Logger, j *database.ActionJob) (err error) { @@ -319,7 +320,7 @@ func (r *actionRunner) handleWebhook(ctx context.Context, j *database.ActionJob) IncludeResults: w.IncludeResults, } - return sendWebhookNotification(ctx, w.URL, args) + return sendWebhookNotification(ctx, w.URL, args, r.logger) } func (r *actionRunner) handleSlackWebhook(ctx context.Context, j *database.ActionJob) error { @@ -355,7 +356,7 @@ func (r *actionRunner) handleSlackWebhook(ctx context.Context, j *database.Actio IncludeResults: w.IncludeResults, } - return sendSlackNotification(ctx, w.URL, args) + return sendSlackNotification(ctx, r.logger, w.URL, args) } type StatusCodeError struct { diff --git a/internal/codemonitors/background/workers_test.go b/internal/codemonitors/background/workers_test.go index 7d26d26ed1e4b..f0e94efb1ed63 100644 --- a/internal/codemonitors/background/workers_test.go +++ b/internal/codemonitors/background/workers_test.go @@ -85,7 +85,7 @@ func TestActionRunner(t *testing.T) { record, err := ts.GetActionJob(ctx, 1) require.NoError(t, err) - a := actionRunner{s} + a := actionRunner{CodeMonitorStore: s, logger: logtest.Scoped(t)} err = a.Handle(ctx, logtest.Scoped(t), record) require.NoError(t, err) diff --git a/internal/database/external_services.go b/internal/database/external_services.go index e8b3dae8bd72a..be3bc681d1512 100644 --- a/internal/database/external_services.go +++ b/internal/database/external_services.go @@ -160,8 +160,9 @@ type externalServiceStore struct { func (e *externalServiceStore) copy() *externalServiceStore { return &externalServiceStore{ - Store: e.Store, - key: e.key, + Store: e.Store, + key: e.key, + logger: e.logger, } } @@ -305,17 +306,17 @@ type ValidateExternalServiceConfigOptions struct { AuthProviders []schema.AuthProviders } -type ValidateExternalServiceConfigFunc = func(ctx context.Context, db DB, opt ValidateExternalServiceConfigOptions) (normalized []byte, err error) +type ValidateExternalServiceConfigFunc = func(ctx context.Context, db DB, opt ValidateExternalServiceConfigOptions, logger log.Logger) (normalized []byte, err error) // ValidateExternalServiceConfig is the default non-enterprise version of our validation function var ValidateExternalServiceConfig = MakeValidateExternalServiceConfigFunc(nil, nil, nil, nil, nil) type ( GitHubValidatorFunc func(DB, *types.GitHubConnection) error - GitLabValidatorFunc func(*schema.GitLabConnection, []schema.AuthProviders) error - BitbucketServerValidatorFunc func(*schema.BitbucketServerConnection) error + GitLabValidatorFunc func(*schema.GitLabConnection, []schema.AuthProviders, log.Logger) error + BitbucketServerValidatorFunc func(*schema.BitbucketServerConnection, log.Logger) error PerforceValidatorFunc func(*schema.PerforceConnection) error - AzureDevOpsValidatorFunc func(connection *schema.AzureDevOpsConnection) error + AzureDevOpsValidatorFunc func(connection *schema.AzureDevOpsConnection, logger log.Logger) error ) func MakeValidateExternalServiceConfigFunc( @@ -325,7 +326,9 @@ func MakeValidateExternalServiceConfigFunc( perforceValidators []PerforceValidatorFunc, azureDevOpsValidators []AzureDevOpsValidatorFunc, ) ValidateExternalServiceConfigFunc { - return func(ctx context.Context, db DB, opt ValidateExternalServiceConfigOptions) (normalized []byte, err error) { + return func(ctx context.Context, db DB, opt ValidateExternalServiceConfigOptions, logger log.Logger) (normalized []byte, err error) { + logger = logger.Scoped("ValidateExternalServiceConfig") + ext, ok := ExternalServiceKinds[opt.Kind] if !ok { return nil, errors.Errorf("invalid external service kind: %s", opt.Kind) @@ -388,14 +391,14 @@ func MakeValidateExternalServiceConfigFunc( if err = jsoniter.Unmarshal(normalized, &c); err != nil { return nil, err } - err = validateGitLabConnection(gitLabValidators, opt.ExternalServiceID, &c, opt.AuthProviders) + err = validateGitLabConnection(gitLabValidators, opt.ExternalServiceID, &c, opt.AuthProviders, logger) case extsvc.KindBitbucketServer: var c schema.BitbucketServerConnection if err = jsoniter.Unmarshal(normalized, &c); err != nil { return nil, err } - err = validateBitbucketServerConnection(bitbucketServerValidators, opt.ExternalServiceID, &c) + err = validateBitbucketServerConnection(bitbucketServerValidators, opt.ExternalServiceID, &c, logger) case extsvc.KindBitbucketCloud: var c schema.BitbucketCloudConnection @@ -414,7 +417,7 @@ func MakeValidateExternalServiceConfigFunc( if err = jsoniter.Unmarshal(normalized, &c); err != nil { return nil, err } - err = validateAzureDevOpsConnection(azureDevOpsValidators, opt.ExternalServiceID, &c) + err = validateAzureDevOpsConnection(azureDevOpsValidators, opt.ExternalServiceID, &c, logger) case extsvc.KindOther: var c schema.OtherExternalServiceConnection if err = jsoniter.Unmarshal(normalized, &c); err != nil { @@ -480,18 +483,18 @@ func validateGitHubConnection(db DB, githubValidators []GitHubValidatorFunc, id return err } -func validateGitLabConnection(gitLabValidators []GitLabValidatorFunc, _ int64, c *schema.GitLabConnection, ps []schema.AuthProviders) error { +func validateGitLabConnection(gitLabValidators []GitLabValidatorFunc, _ int64, c *schema.GitLabConnection, ps []schema.AuthProviders, logger log.Logger) error { var err error for _, validate := range gitLabValidators { - err = errors.Append(err, validate(c, ps)) + err = errors.Append(err, validate(c, ps, logger)) } return err } -func validateAzureDevOpsConnection(azureDevOpsValidators []AzureDevOpsValidatorFunc, _ int64, c *schema.AzureDevOpsConnection) error { +func validateAzureDevOpsConnection(azureDevOpsValidators []AzureDevOpsValidatorFunc, _ int64, c *schema.AzureDevOpsConnection, logger log.Logger) error { var err error for _, validate := range azureDevOpsValidators { - err = errors.Append(err, validate(c)) + err = errors.Append(err, validate(c, logger)) } if c.Projects == nil && c.Orgs == nil { err = errors.Append(err, errors.New("either 'projects' or 'orgs' must be set")) @@ -499,10 +502,10 @@ func validateAzureDevOpsConnection(azureDevOpsValidators []AzureDevOpsValidatorF return err } -func validateBitbucketServerConnection(bitbucketServerValidators []BitbucketServerValidatorFunc, _ int64, c *schema.BitbucketServerConnection) error { +func validateBitbucketServerConnection(bitbucketServerValidators []BitbucketServerValidatorFunc, _ int64, c *schema.BitbucketServerConnection, logger log.Logger) error { var err error for _, validate := range bitbucketServerValidators { - err = errors.Append(err, validate(c)) + err = errors.Append(err, validate(c, logger)) } if c.Repos == nil && c.RepositoryQuery == nil && c.ProjectKeys == nil { @@ -539,7 +542,7 @@ func (e *externalServiceStore) Create(ctx context.Context, confGet func() *conf. Kind: es.Kind, Config: rawConfig, AuthProviders: confGet().AuthProviders, - }) + }, e.logger) if err != nil { return err } @@ -627,7 +630,7 @@ func (e *externalServiceStore) Upsert(ctx context.Context, svcs ...*types.Extern Kind: s.Kind, Config: rawConfig, AuthProviders: authProviders, - }) + }, e.logger) if err != nil { return errors.Wrapf(err, "validating service of kind %q", s.Kind) } @@ -867,7 +870,7 @@ func (e *externalServiceStore) Update(ctx context.Context, ps []schema.AuthProvi Kind: externalService.Kind, Config: unredactedConfig, AuthProviders: ps, - }) + }, e.logger) if err != nil { return err } diff --git a/internal/database/external_services_external_test.go b/internal/database/external_services_external_test.go index 1361d4f24a50a..c695e6e217654 100644 --- a/internal/database/external_services_external_test.go +++ b/internal/database/external_services_external_test.go @@ -5,6 +5,8 @@ import ( "fmt" "testing" + "github.com/sourcegraph/log/logtest" + "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/database/dbmocks" "github.com/sourcegraph/sourcegraph/internal/extsvc" @@ -115,7 +117,7 @@ func TestExternalServicesStore_ValidateConfig(t *testing.T) { _, err := database.ValidateExternalServiceConfig(context.Background(), dbm, database.ValidateExternalServiceConfigOptions{ Kind: test.kind, Config: test.config, - }) + }, logtest.Scoped(t)) gotErr := fmt.Sprintf("%v", err) if gotErr != test.wantErr { t.Errorf("error: want %q but got %q", test.wantErr, gotErr) diff --git a/internal/database/external_services_test.go b/internal/database/external_services_test.go index 2a3a47f896324..619490c563201 100644 --- a/internal/database/external_services_test.go +++ b/internal/database/external_services_test.go @@ -655,7 +655,7 @@ func TestExternalServiceStore_Delete_WithSyncJobs(t *testing.T) { t.Parallel() logger := logtest.Scoped(t) db := NewDB(logger, dbtest.NewDB(t)) - store := &externalServiceStore{Store: basestore.NewWithHandle(db.Handle())} + store := &externalServiceStore{Store: basestore.NewWithHandle(db.Handle()), logger: logger} ctx := context.Background() // Create a new external service @@ -1142,7 +1142,7 @@ func TestExternalServiceStore_HasRunningSyncJobs(t *testing.T) { t.Parallel() logger := logtest.Scoped(t) db := NewDB(logger, dbtest.NewDB(t)) - store := &externalServiceStore{Store: basestore.NewWithHandle(db.Handle())} + store := &externalServiceStore{Store: basestore.NewWithHandle(db.Handle()), logger: logger} ctx := context.Background() // Create a new external service diff --git a/internal/extsvc/azuredevops/client.go b/internal/extsvc/azuredevops/client.go index e7e15398576b4..6c82ceec70173 100644 --- a/internal/extsvc/azuredevops/client.go +++ b/internal/extsvc/azuredevops/client.go @@ -66,30 +66,34 @@ type client struct { auth auth.Authenticator waitForRateLimit bool maxRateLimitRetries int + logger log.Logger } // NewClient returns an authenticated AzureDevOps API client with // the provided configuration. If a nil httpClient is provided, http.DefaultClient // will be used. -func NewClient(urn string, url string, auth auth.Authenticator, httpClient httpcli.Doer) (Client, error) { +func NewClient(urn string, url string, auth auth.Authenticator, httpClient httpcli.Doer, logger log.Logger) (Client, error) { + logger = logger.Scoped("AzureDevOpsClient") + u, err := urlx.Parse(url) if err != nil { return nil, err } if httpClient == nil { - httpClient = httpcli.ExternalDoer + httpClient = httpcli.ExternalDoer(logger) } return &client{ httpClient: httpClient, URL: u, - internalRateLimiter: ratelimit.NewInstrumentedLimiter(urn, ratelimit.NewGlobalRateLimiter(log.Scoped("AzureDevOpsClient"), urn)), + internalRateLimiter: ratelimit.NewInstrumentedLimiter(urn, ratelimit.NewGlobalRateLimiter(logger, urn)), externalRateLimiter: ratelimit.DefaultMonitorRegistry.GetOrSet(url, auth.Hash(), "rest", &ratelimit.Monitor{HeaderPrefix: "X-"}), auth: auth, urn: urn, waitForRateLimit: true, maxRateLimitRetries: 2, + logger: logger, }, nil } @@ -186,7 +190,7 @@ func (c *client) WithAuthenticator(a auth.Authenticator) (Client, error) { return nil, errors.Errorf("authenticator type unsupported for Azure DevOps clients: %s", a) } - return NewClient(c.urn, c.URL.String(), a, c.httpClient) + return NewClient(c.urn, c.URL.String(), a, c.httpClient, c.logger) } func (c *client) SetWaitForRateLimit(wait bool) { diff --git a/internal/extsvc/azuredevops/client_test.go b/internal/extsvc/azuredevops/client_test.go index 337c9d5de1333..f1c46b240202f 100644 --- a/internal/extsvc/azuredevops/client_test.go +++ b/internal/extsvc/azuredevops/client_test.go @@ -13,6 +13,7 @@ import ( "time" "github.com/dnaeon/go-vcr/cassette" + "github.com/sourcegraph/log/logtest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/time/rate" @@ -52,6 +53,7 @@ func NewTestClient(t testing.TB, name string, update bool) (Client, func()) { Password: os.Getenv("AZURE_DEV_OPS_TOKEN"), }, hc, + logtest.Scoped(t), ) if err != nil { t.Fatal(err) @@ -139,7 +141,7 @@ func TestRateLimitRetry(t *testing.T) { MockVisualStudioAppURL = "" }) a := &auth.BasicAuth{Username: "test", Password: "test"} - c, err := NewClient("test", srv.URL, a, httpcli.TestExternalDoer) + c, err := NewClient("test", srv.URL, a, httpcli.TestExternalDoer, logtest.Scoped(t)) c.(*client).internalRateLimiter = ratelimit.NewInstrumentedLimiter("azuredevops", rate.NewLimiter(100, 10)) require.NoError(t, err) c.SetWaitForRateLimit(tt.waitForRateLimit) diff --git a/internal/extsvc/bitbucketcloud/BUILD.bazel b/internal/extsvc/bitbucketcloud/BUILD.bazel index c9b6b6d219200..57b271275260a 100644 --- a/internal/extsvc/bitbucketcloud/BUILD.bazel +++ b/internal/extsvc/bitbucketcloud/BUILD.bazel @@ -63,6 +63,7 @@ go_test( "//schema", "@com_github_google_go_cmp//cmp", "@com_github_grafana_regexp//:regexp", + "@com_github_sourcegraph_log//logtest", "@com_github_stretchr_testify//assert", "@org_golang_x_time//rate", ], diff --git a/internal/extsvc/bitbucketcloud/client.go b/internal/extsvc/bitbucketcloud/client.go index b2079597505b9..b26e21d7d5e57 100644 --- a/internal/extsvc/bitbucketcloud/client.go +++ b/internal/extsvc/bitbucketcloud/client.go @@ -79,13 +79,13 @@ type client struct { // NewClient creates a new Bitbucket Cloud API client from the given external // service configuration. If a nil httpClient is provided, http.DefaultClient // will be used. -func NewClient(urn string, config *schema.BitbucketCloudConnection, httpClient httpcli.Doer) (Client, error) { - return newClient(urn, config, httpClient) +func NewClient(urn string, config *schema.BitbucketCloudConnection, httpClient httpcli.Doer, logger log.Logger) (Client, error) { + return newClient(urn, config, httpClient, logger) } -func newClient(urn string, config *schema.BitbucketCloudConnection, httpClient httpcli.Doer) (*client, error) { +func newClient(urn string, config *schema.BitbucketCloudConnection, httpClient httpcli.Doer, logger log.Logger) (*client, error) { if httpClient == nil { - httpClient = httpcli.ExternalDoer + httpClient = httpcli.ExternalDoer(logger) } httpClient = requestCounter.Doer(httpClient, func(u *url.URL) string { diff --git a/internal/extsvc/bitbucketcloud/main_test.go b/internal/extsvc/bitbucketcloud/main_test.go index 9090e1749e59e..197d6af0be618 100644 --- a/internal/extsvc/bitbucketcloud/main_test.go +++ b/internal/extsvc/bitbucketcloud/main_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/grafana/regexp" + "github.com/sourcegraph/log/logtest" "golang.org/x/time/rate" bbtest "github.com/sourcegraph/sourcegraph/internal/extsvc/bitbucketcloud/testing" @@ -68,7 +69,7 @@ func newTestClient(t testing.TB) *client { ApiURL: "https://api.bitbucket.org", Username: bbtest.GetenvTestBitbucketCloudUsername(), AppPassword: os.Getenv("BITBUCKET_CLOUD_APP_PASSWORD"), - }, hc) + }, hc, logtest.Scoped(t)) if err != nil { t.Fatal(err) } diff --git a/internal/extsvc/bitbucketserver/BUILD.bazel b/internal/extsvc/bitbucketserver/BUILD.bazel index 6b738ef84b760..b07d266ae5920 100644 --- a/internal/extsvc/bitbucketserver/BUILD.bazel +++ b/internal/extsvc/bitbucketserver/BUILD.bazel @@ -52,6 +52,7 @@ go_test( "@com_github_google_go_cmp//cmp", "@com_github_inconshreveable_log15//:log15", "@com_github_sergi_go_diff//diffmatchpatch", + "@com_github_sourcegraph_log//logtest", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", "@org_golang_x_time//rate", diff --git a/internal/extsvc/bitbucketserver/client.go b/internal/extsvc/bitbucketserver/client.go index 57d1ac3f5b15b..9e77528f4d715 100644 --- a/internal/extsvc/bitbucketserver/client.go +++ b/internal/extsvc/bitbucketserver/client.go @@ -63,8 +63,8 @@ type Client struct { // NewClient returns an authenticated Bitbucket Server API client with // the provided configuration. If a nil httpClient is provided, http.DefaultClient // will be used. -func NewClient(urn string, config *schema.BitbucketServerConnection, httpClient httpcli.Doer) (*Client, error) { - client, err := newClient(urn, config, httpClient) +func NewClient(urn string, config *schema.BitbucketServerConnection, httpClient httpcli.Doer, logger log.Logger) (*Client, error) { + client, err := newClient(urn, config, httpClient, logger.Scoped("BitbucketServerClient")) if err != nil { return nil, err } @@ -91,14 +91,16 @@ func NewClient(urn string, config *schema.BitbucketServerConnection, httpClient return client, nil } -func newClient(urn string, config *schema.BitbucketServerConnection, httpClient httpcli.Doer) (*Client, error) { +func newClient(urn string, config *schema.BitbucketServerConnection, httpClient httpcli.Doer, logger log.Logger) (*Client, error) { u, err := url.Parse(config.Url) if err != nil { return nil, err } + logger = logger.Scoped("BitbucketServerClient") + if httpClient == nil { - httpClient = httpcli.ExternalDoer + httpClient = httpcli.ExternalDoer(logger) } httpClient = requestCounter.Doer(httpClient, categorize) @@ -106,7 +108,7 @@ func newClient(urn string, config *schema.BitbucketServerConnection, httpClient httpClient: httpClient, URL: u, // Default limits are defined in extsvc.GetLimitFromConfig - rateLimit: ratelimit.NewInstrumentedLimiter(urn, ratelimit.NewGlobalRateLimiter(log.Scoped("BitbucketServerClient"), urn)), + rateLimit: ratelimit.NewInstrumentedLimiter(urn, ratelimit.NewGlobalRateLimiter(logger, urn)), }, nil } diff --git a/internal/extsvc/bitbucketserver/client_test.go b/internal/extsvc/bitbucketserver/client_test.go index 3adbbcc539ac8..bc5c5a1fb14d2 100644 --- a/internal/extsvc/bitbucketserver/client_test.go +++ b/internal/extsvc/bitbucketserver/client_test.go @@ -22,6 +22,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/inconshreveable/log15" //nolint:logging // TODO move all logging to sourcegraph/log "github.com/sergi/go-diff/diffmatchpatch" + "github.com/sourcegraph/log/logtest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/time/rate" @@ -92,7 +93,7 @@ func TestClientKeepsBaseURLPath(t *testing.T) { srvURL, err := url.JoinPath(srv.URL, "/testpath") require.NoError(t, err) bbConf := &schema.BitbucketServerConnection{Url: srvURL} - client, err := NewClient("test", bbConf, httpcli.TestExternalDoer) + client, err := NewClient("test", bbConf, httpcli.TestExternalDoer, logtest.Scoped(t)) require.NoError(t, err) client.rateLimit = ratelimit.NewInstrumentedLimiter("bitbucket", rate.NewLimiter(100, 10)) @@ -162,7 +163,7 @@ func TestUserFilters(t *testing.T) { } func TestClient_Users(t *testing.T) { - cli := NewTestClient(t, "Users", *update) + cli := NewTestClient(t, logtest.Scoped(t), "Users", *update) timeout, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) defer cancel() @@ -338,7 +339,7 @@ func TestClient_Users(t *testing.T) { } func TestClient_LabeledRepos(t *testing.T) { - cli := NewTestClient(t, "LabeledRepos", *update) + cli := NewTestClient(t, logtest.Scoped(t), "LabeledRepos", *update) // We have archived label on bitbucket.sgdev.org with a repo in it. repos, _, err := cli.LabeledRepos(context.Background(), nil, "archived") @@ -424,7 +425,7 @@ func TestClient_LoadPullRequest(t *testing.T) { tc := tc t.Run(tc.name, func(t *testing.T) { name := "PullRequests-" + strings.ReplaceAll(tc.name, " ", "-") - cli := NewTestClient(t, name, *update) + cli := NewTestClient(t, logtest.Scoped(t), name, *update) if tc.ctx == nil { tc.ctx = context.Background() @@ -573,7 +574,7 @@ func TestClient_CreatePullRequest(t *testing.T) { tc := tc t.Run(tc.name, func(t *testing.T) { name := "CreatePullRequest-" + strings.ReplaceAll(tc.name, " ", "-") - cli := NewTestClient(t, name, *update) + cli := NewTestClient(t, logtest.Scoped(t), name, *update) if tc.ctx == nil { tc.ctx = context.Background() @@ -716,7 +717,7 @@ func TestClient_FetchDefaultReviewers(t *testing.T) { tc := tc t.Run(tc.name, func(t *testing.T) { name := "FetchDefaultReviewers-" + strings.ReplaceAll(tc.name, " ", "-") - cli := NewTestClient(t, name, *update) + cli := NewTestClient(t, logtest.Scoped(t), name, *update) if tc.ctx == nil { tc.ctx = context.Background() @@ -798,7 +799,7 @@ func TestClient_DeclinePullRequest(t *testing.T) { tc := tc t.Run(tc.name, func(t *testing.T) { name := "DeclinePullRequest-" + strings.ReplaceAll(tc.name, " ", "-") - cli := NewTestClient(t, name, *update) + cli := NewTestClient(t, logtest.Scoped(t), name, *update) if tc.ctx == nil { tc.ctx = context.Background() @@ -830,7 +831,7 @@ func TestClient_LoadPullRequestActivities(t *testing.T) { instanceURL = "https://bitbucket.sgdev.org" } - cli := NewTestClient(t, "PullRequestActivities", *update) + cli := NewTestClient(t, logtest.Scoped(t), "PullRequestActivities", *update) timeout, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) defer cancel() @@ -952,7 +953,7 @@ func TestClient_CreatePullRequestComment(t *testing.T) { tc := tc t.Run(tc.name, func(t *testing.T) { name := "CreatePullRequestComment-" + strings.ReplaceAll(tc.name, " ", "-") - cli := NewTestClient(t, name, *update) + cli := NewTestClient(t, logtest.Scoped(t), name, *update) if tc.ctx == nil { tc.ctx = context.Background() @@ -1039,7 +1040,7 @@ func TestClient_MergePullRequest(t *testing.T) { t.Run(tc.name, func(t *testing.T) { name := "MergePullRequest-" + strings.ReplaceAll(tc.name, " ", "-") - cli := NewTestClient(t, name, *update) + cli := NewTestClient(t, logtest.Scoped(t), name, *update) if tc.ctx == nil { tc.ctx = context.Background() @@ -1070,7 +1071,7 @@ func TestClient_MergePullRequest(t *testing.T) { // dependent on the user token supplied. The current golden files are generated // from using the account zoom@sourcegraph.com on bitbucket.sgdev.org. func TestClient_RepoIDs(t *testing.T) { - cli := NewTestClient(t, "RepoIDs", *update) + cli := NewTestClient(t, logtest.Scoped(t), "RepoIDs", *update) ids, err := cli.RepoIDs(context.Background(), "READ") if err != nil { @@ -1115,7 +1116,7 @@ func TestAuth(t *testing.T) { client, err := NewClient("urn", &schema.BitbucketServerConnection{ Url: "http://example.com/", Token: "foo", - }, nil) + }, nil, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -1133,7 +1134,7 @@ func TestAuth(t *testing.T) { Url: "http://example.com/", Username: "foo", Password: "bar", - }, nil) + }, nil, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -1155,7 +1156,7 @@ func TestAuth(t *testing.T) { SigningKey: "this is an invalid key", }, }, - }, nil); err == nil { + }, nil, logtest.Scoped(t)); err == nil { t.Error("unexpected nil error") } }) @@ -1179,7 +1180,7 @@ func TestAuth(t *testing.T) { SigningKey: signingKey, }, }, - }, nil) + }, nil, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -1275,7 +1276,7 @@ func TestClient_WithAuthenticator(t *testing.T) { func TestClient_GetVersion(t *testing.T) { fixture := "GetVersion" - cli := NewTestClient(t, fixture, *update) + cli := NewTestClient(t, logtest.Scoped(t), fixture, *update) have, err := cli.GetVersion(context.Background()) if err != nil { @@ -1291,7 +1292,7 @@ func TestClient_CreateFork(t *testing.T) { ctx := context.Background() fixture := "CreateFork" - cli := NewTestClient(t, fixture, *update) + cli := NewTestClient(t, logtest.Scoped(t), fixture, *update) have, err := cli.Fork(ctx, "SGDEMO", "go", CreateForkInput{}) assert.Nil(t, err) @@ -1303,7 +1304,7 @@ func TestClient_CreateFork(t *testing.T) { } func TestClient_ProjectRepos(t *testing.T) { - cli := NewTestClient(t, "ProjectRepos", *update) + cli := NewTestClient(t, logtest.Scoped(t), "ProjectRepos", *update) // Empty project key should cause an error _, err := cli.ProjectRepos(context.Background(), "") diff --git a/internal/extsvc/bitbucketserver/testing.go b/internal/extsvc/bitbucketserver/testing.go index 2881a9517ec2d..a15f9b4a8684c 100644 --- a/internal/extsvc/bitbucketserver/testing.go +++ b/internal/extsvc/bitbucketserver/testing.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/dnaeon/go-vcr/cassette" + "github.com/sourcegraph/log" "golang.org/x/time/rate" "github.com/sourcegraph/sourcegraph/internal/httpcli" @@ -19,7 +20,7 @@ import ( // NewTestClient returns a bitbucketserver.Client that records its interactions // to testdata/vcr/. -func NewTestClient(t testing.TB, name string, update bool) *Client { +func NewTestClient(t testing.TB, logger log.Logger, name string, update bool) *Client { t.Helper() cassete := filepath.Join("testdata/vcr/", normalize(name)) @@ -49,7 +50,7 @@ func NewTestClient(t testing.TB, name string, update bool) *Client { Url: instanceURL, } - cli, err := NewClient("urn", c, hc) + cli, err := NewClient("urn", c, hc, logger) if err != nil { t.Fatal(err) } diff --git a/internal/extsvc/gerrit/BUILD.bazel b/internal/extsvc/gerrit/BUILD.bazel index d113b5833cd68..6828acdf1e5e1 100644 --- a/internal/extsvc/gerrit/BUILD.bazel +++ b/internal/extsvc/gerrit/BUILD.bazel @@ -44,6 +44,7 @@ go_test( "//internal/testutil", "//lib/errors", "@com_github_dnaeon_go_vcr//cassette", + "@com_github_sourcegraph_log//logtest", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", "@org_golang_x_time//rate", diff --git a/internal/extsvc/gerrit/account.go b/internal/extsvc/gerrit/account.go index 1c1bbcfd47bf9..df000b1a660b4 100644 --- a/internal/extsvc/gerrit/account.go +++ b/internal/extsvc/gerrit/account.go @@ -5,6 +5,8 @@ import ( "encoding/json" "net/url" + "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/internal/encryption" "github.com/sourcegraph/sourcegraph/internal/extsvc" ) @@ -60,14 +62,14 @@ func SetExternalAccountData(data *extsvc.AccountData, usr *Account, creds *Accou return nil } -var MockVerifyAccount func(context.Context, *url.URL, *AccountCredentials) (*Account, error) +var MockVerifyAccount func(context.Context, log.Logger, *url.URL, *AccountCredentials) (*Account, error) -func VerifyAccount(ctx context.Context, u *url.URL, creds *AccountCredentials) (*Account, error) { +func VerifyAccount(ctx context.Context, logger log.Logger, u *url.URL, creds *AccountCredentials) (*Account, error) { if MockVerifyAccount != nil { - return MockVerifyAccount(ctx, u, creds) + return MockVerifyAccount(ctx, logger, u, creds) } - client, err := NewClient("", u, creds, nil) + client, err := NewClient("", u, creds, nil, logger) if err != nil { return nil, err } diff --git a/internal/extsvc/gerrit/client.go b/internal/extsvc/gerrit/client.go index db6c2358ce6d0..28988ab04d66a 100644 --- a/internal/extsvc/gerrit/client.go +++ b/internal/extsvc/gerrit/client.go @@ -59,9 +59,11 @@ type Client interface { // NewClient returns an authenticated Gerrit API client with // the provided configuration. If a nil httpClient is provided, httpcli.ExternalDoer // will be used. -func NewClient(urn string, url *url.URL, creds *AccountCredentials, httpClient httpcli.Doer) (Client, error) { +func NewClient(urn string, url *url.URL, creds *AccountCredentials, httpClient httpcli.Doer, logger log.Logger) (Client, error) { + logger = logger.Scoped("GerritClient") + if httpClient == nil { - httpClient = httpcli.ExternalDoer + httpClient = httpcli.ExternalDoer(logger) } auther := &auth.BasicAuth{ @@ -72,7 +74,7 @@ func NewClient(urn string, url *url.URL, creds *AccountCredentials, httpClient h return &client{ httpClient: httpClient, URL: url, - rateLimit: ratelimit.NewInstrumentedLimiter(urn, ratelimit.NewGlobalRateLimiter(log.Scoped("GerritClient"), urn)), + rateLimit: ratelimit.NewInstrumentedLimiter(urn, ratelimit.NewGlobalRateLimiter(logger, urn)), auther: auther, }, nil } diff --git a/internal/extsvc/gerrit/externalaccount/BUILD.bazel b/internal/extsvc/gerrit/externalaccount/BUILD.bazel index 61b1f268c1755..9011c123ea253 100644 --- a/internal/extsvc/gerrit/externalaccount/BUILD.bazel +++ b/internal/extsvc/gerrit/externalaccount/BUILD.bazel @@ -10,5 +10,6 @@ go_library( "//internal/database", "//internal/extsvc", "//internal/extsvc/gerrit", + "@com_github_sourcegraph_log//:log", ], ) diff --git a/internal/extsvc/gerrit/externalaccount/externalaccount.go b/internal/extsvc/gerrit/externalaccount/externalaccount.go index a3b9611ab0a76..8c6bab15d52d8 100644 --- a/internal/extsvc/gerrit/externalaccount/externalaccount.go +++ b/internal/extsvc/gerrit/externalaccount/externalaccount.go @@ -6,12 +6,14 @@ import ( "net/url" "strconv" + "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/extsvc" "github.com/sourcegraph/sourcegraph/internal/extsvc/gerrit" ) -func AddGerritExternalAccount(ctx context.Context, db database.DB, userID int32, serviceID string, accountDetails string) (err error) { +func AddGerritExternalAccount(ctx context.Context, db database.DB, logger log.Logger, userID int32, serviceID string, accountDetails string) (err error) { var accountCredentials gerrit.AccountCredentials err = json.Unmarshal([]byte(accountDetails), &accountCredentials) if err != nil { @@ -24,7 +26,7 @@ func AddGerritExternalAccount(ctx context.Context, db database.DB, userID int32, } serviceURL = extsvc.NormalizeBaseURL(serviceURL) - gerritAccount, err := gerrit.VerifyAccount(ctx, serviceURL, &accountCredentials) + gerritAccount, err := gerrit.VerifyAccount(ctx, logger, serviceURL, &accountCredentials) if err != nil { return err } diff --git a/internal/extsvc/gerrit/main_test.go b/internal/extsvc/gerrit/main_test.go index 286c7c2efea15..697b9cedca5e5 100644 --- a/internal/extsvc/gerrit/main_test.go +++ b/internal/extsvc/gerrit/main_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/dnaeon/go-vcr/cassette" + "github.com/sourcegraph/log/logtest" "golang.org/x/time/rate" "github.com/sourcegraph/sourcegraph/internal/httpcli" @@ -49,7 +50,7 @@ func NewTestClient(t testing.TB, name string, update bool) (Client, func()) { cli, err := NewClient("urn", u, &AccountCredentials{ Username: os.Getenv("GERRIT_USERNAME"), Password: os.Getenv("GERRIT_PASSWORD"), - }, hc) + }, hc, logtest.Scoped(t)) if err != nil { t.Fatal(err) } diff --git a/internal/extsvc/github/v3.go b/internal/extsvc/github/v3.go index 2fba327a20c2d..2931ed2c95a6b 100644 --- a/internal/extsvc/github/v3.go +++ b/internal/extsvc/github/v3.go @@ -88,12 +88,18 @@ func NewV3SearchClient(logger log.Logger, urn string, apiURL *url.URL, a auth.Au } func newV3Client(logger log.Logger, urn string, apiURL *url.URL, a auth.Authenticator, resource string, cli httpcli.Doer) *V3Client { + logger = logger.Scoped("github.v3"). + With( + log.String("urn", urn), + log.String("resource", resource), + ) + apiURL = canonicalizedURL(apiURL) if gitHubDisable { cli = disabledClient{} } if cli == nil { - cli = httpcli.ExternalDoer + cli = httpcli.ExternalDoer(logger) } cli = requestCounter.Doer(cli, func(u *url.URL) string { @@ -116,11 +122,7 @@ func newV3Client(logger log.Logger, urn string, apiURL *url.URL, a auth.Authenti rlm := ratelimit.DefaultMonitorRegistry.GetOrSet(apiURL.String(), tokenHash, resource, &ratelimit.Monitor{HeaderPrefix: "X-"}) return &V3Client{ - log: logger.Scoped("github.v3"). - With( - log.String("urn", urn), - log.String("resource", resource), - ), + log: logger, urn: urn, apiURL: apiURL, githubDotCom: URLIsGitHubDotCom(apiURL), diff --git a/internal/extsvc/github/v4.go b/internal/extsvc/github/v4.go index c2c8e2237b9f4..c3ae8a3f038e7 100644 --- a/internal/extsvc/github/v4.go +++ b/internal/extsvc/github/v4.go @@ -68,13 +68,18 @@ type V4Client struct { // // apiURL must point to the base URL of the GitHub API. See the docstring for // V4Client.apiURL. -func NewV4Client(urn string, apiURL *url.URL, a auth.Authenticator, cli httpcli.Doer) *V4Client { +func NewV4Client(urn string, apiURL *url.URL, a auth.Authenticator, cli httpcli.Doer, logger log.Logger) *V4Client { + logger = logger.Scoped("github.v4").With( + log.String("urn", urn), + log.String("apiURL", apiURL.String()), + ) + apiURL = canonicalizedURL(apiURL) if gitHubDisable { cli = disabledClient{} } if cli == nil { - cli = httpcli.ExternalDoer + cli = httpcli.ExternalDoer(logger) } cli = requestCounter.Doer(cli, func(u *url.URL) string { @@ -97,7 +102,7 @@ func NewV4Client(urn string, apiURL *url.URL, a auth.Authenticator, cli httpcli. rlm := ratelimit.DefaultMonitorRegistry.GetOrSet(apiURL.String(), tokenHash, "graphql", &ratelimit.Monitor{HeaderPrefix: "X-"}) return &V4Client{ - log: log.Scoped("github.v4"), + log: logger, urn: urn, apiURL: apiURL, githubDotCom: URLIsGitHubDotCom(apiURL), @@ -114,7 +119,7 @@ func NewV4Client(urn string, apiURL *url.URL, a auth.Authenticator, cli httpcli. // the current V4Client, except authenticated as the GitHub user with the given // authenticator instance (most likely a token). func (c *V4Client) WithAuthenticator(a auth.Authenticator) *V4Client { - return NewV4Client(c.urn, c.apiURL, a, c.httpClient) + return NewV4Client(c.urn, c.apiURL, a, c.httpClient, c.log) } // ExternalRateLimiter exposes the rate limit monitor. diff --git a/internal/extsvc/github/v4_test.go b/internal/extsvc/github/v4_test.go index 798f2e241beb3..36330129f10f2 100644 --- a/internal/extsvc/github/v4_test.go +++ b/internal/extsvc/github/v4_test.go @@ -17,6 +17,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/sourcegraph/log/logtest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/time/rate" @@ -154,7 +155,7 @@ func TestV4Client_RateLimitRetry(t *testing.T) { transport := http.DefaultTransport.(*http.Transport).Clone() transport.DisableKeepAlives = true // Disable keep-alives otherwise the read of the request body is cached cli := &http.Client{Transport: transport} - client := NewV4Client("test", srvURL, nil, cli) + client := NewV4Client("test", srvURL, nil, cli, logtest.Scoped(t)) client.internalRateLimiter = ratelimit.NewInstrumentedLimiter("githubv4", rate.NewLimiter(100, 10)) client.githubDotCom = true // Otherwise it will make an extra request to determine GH version _, err = client.SearchRepos(ctx, SearchReposParams{Query: "test"}) @@ -229,7 +230,7 @@ func TestV4Client_RequestGraphQL_RequestUnmutated(t *testing.T) { // Now we create a client to talk to our test server with the API path // appended. - client := NewV4Client("test", apiURL, nil, cli) + client := NewV4Client("test", apiURL, nil, cli, logtest.Scoped(t)) // Now we send a request that should run into rate limiting error. err = client.requestGraphQL(ctx, query, vars, &result) @@ -944,6 +945,7 @@ func TestV4Client_WithAuthenticator(t *testing.T) { oldClient := &V4Client{ apiURL: uri, auth: &auth.OAuthBearerToken{Token: "old_token"}, + log: logtest.Scoped(t), } newToken := &auth.OAuthBearerToken{Token: "new_token"} @@ -971,7 +973,7 @@ func newV4Client(t testing.TB, name string) (*V4Client, func()) { t.Fatal(err) } - cli := NewV4Client("Test", uri, vcrToken, doer) + cli := NewV4Client("Test", uri, vcrToken, doer, logtest.Scoped(t)) cli.internalRateLimiter = ratelimit.NewInstrumentedLimiter("githubv4", rate.NewLimiter(100, 10)) return cli, save @@ -992,7 +994,7 @@ func newEnterpriseV4Client(t testing.TB, name string) (*V4Client, func()) { t.Fatal(err) } - cli := NewV4Client("Test", uri, gheToken, doer) + cli := NewV4Client("Test", uri, gheToken, doer, logtest.Scoped(t)) cli.internalRateLimiter = ratelimit.NewInstrumentedLimiter("githubv4", rate.NewLimiter(100, 10)) return cli, save } @@ -1179,7 +1181,7 @@ func TestClient_GetReposByNameWithOwner(t *testing.T) { t.Run(tc.name, func(t *testing.T) { mock := mockHTTPResponseBody{responseBody: tc.mockResponseBody} apiURL := &url.URL{Scheme: "https", Host: "example.com", Path: "/"} - c := NewV4Client("Test", apiURL, nil, &mock) + c := NewV4Client("Test", apiURL, nil, &mock, logtest.Scoped(t)) c.internalRateLimiter = ratelimit.NewInstrumentedLimiter("githubv4", rate.NewLimiter(100, 10)) repos, err := c.GetReposByNameWithOwner(context.Background(), namesWithOwners...) @@ -1231,7 +1233,7 @@ repo8: repository(owner: "sourcegraph", name: "contains.dot") { ... on Repositor mock := mockHTTPResponseBody{responseBody: ""} apiURL := &url.URL{Scheme: "https", Host: "example.com", Path: "/"} - c := NewV4Client("Test", apiURL, nil, &mock) + c := NewV4Client("Test", apiURL, nil, &mock, logtest.Scoped(t)) query, err := c.buildGetReposBatchQuery(context.Background(), repos) if err != nil { t.Fatal(err) diff --git a/internal/extsvc/gitlab/BUILD.bazel b/internal/extsvc/gitlab/BUILD.bazel index 6cc845d19507c..5957bf6ed195c 100644 --- a/internal/extsvc/gitlab/BUILD.bazel +++ b/internal/extsvc/gitlab/BUILD.bazel @@ -89,6 +89,7 @@ go_test( "@com_github_google_go_cmp//cmp", "@com_github_grafana_regexp//:regexp", "@com_github_masterminds_semver//:semver", + "@com_github_sourcegraph_log//logtest", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", "@org_golang_x_time//rate", diff --git a/internal/extsvc/gitlab/client.go b/internal/extsvc/gitlab/client.go index 1b6dcf7f73d1d..e10185bf9d8fc 100644 --- a/internal/extsvc/gitlab/client.go +++ b/internal/extsvc/gitlab/client.go @@ -65,9 +65,14 @@ type CommonOp struct { NoCache bool } -func NewClientProvider(urn string, baseURL *url.URL, cli httpcli.Doer) *ClientProvider { +func NewClientProvider(urn string, baseURL *url.URL, cli httpcli.Doer, logger log.Logger) *ClientProvider { + logger = logger.Scoped("GitLabClientProvider").With( + log.String("urn", urn), + log.String("baseURL", baseURL.String()), + ) + if cli == nil { - cli = httpcli.ExternalDoer + cli = httpcli.ExternalDoer(logger) } cli = requestCounter.Doer(cli, func(u *url.URL) string { // The 3rd component of the Path (/api/v4/XYZ) mostly maps to the type of API diff --git a/internal/extsvc/gitlab/client_test.go b/internal/extsvc/gitlab/client_test.go index 56055482f60c3..88ccd84ebbf5b 100644 --- a/internal/extsvc/gitlab/client_test.go +++ b/internal/extsvc/gitlab/client_test.go @@ -15,6 +15,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/grafana/regexp" + "github.com/sourcegraph/log/logtest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/time/rate" @@ -53,7 +54,7 @@ func createTestProvider(t *testing.T) *ClientProvider { t.Fatal(err) } baseURL, _ := url.Parse("https://gitlab.com/") - provider := NewClientProvider("Test", baseURL, doer) + provider := NewClientProvider("Test", baseURL, doer, logtest.Scoped(t)) return provider } @@ -108,7 +109,7 @@ func TestClient_doWithBaseURL(t *testing.T) { ctx := context.Background() - provider := NewClientProvider("Test", baseURL, doer) + provider := NewClientProvider("Test", baseURL, doer, logtest.Scoped(t)) client := provider.getClient(&auth.OAuthBearerToken{Token: "bad token", RefreshToken: "refresh token", RefreshFunc: func(ctx context.Context, cli httpcli.Doer, obt *auth.OAuthBearerToken) (string, string, time.Time, error) { obt.Token = "refreshed-token" @@ -197,7 +198,7 @@ func TestRateLimitRetry(t *testing.T) { srvURL, err := url.Parse(srv.URL) require.NoError(t, err) - provider := NewClientProvider("Test", srvURL, httpcli.TestExternalDoer) + provider := NewClientProvider("Test", srvURL, httpcli.TestExternalDoer, logtest.Scoped(t)) client := provider.getClient(nil) client.internalRateLimiter = ratelimit.NewInstrumentedLimiter("gitlab", rate.NewLimiter(100, 10)) client.waitForRateLimit = tt.waitForRateLimit diff --git a/internal/extsvc/pagure/BUILD.bazel b/internal/extsvc/pagure/BUILD.bazel index 66387a266f3a4..587923de330cc 100644 --- a/internal/extsvc/pagure/BUILD.bazel +++ b/internal/extsvc/pagure/BUILD.bazel @@ -34,5 +34,6 @@ go_test( deps = [ "//internal/testutil", "@com_github_inconshreveable_log15//:log15", + "@com_github_sourcegraph_log//logtest", ], ) diff --git a/internal/extsvc/pagure/client.go b/internal/extsvc/pagure/client.go index fe4fa2fcbfe96..076240740bb25 100644 --- a/internal/extsvc/pagure/client.go +++ b/internal/extsvc/pagure/client.go @@ -38,14 +38,14 @@ type Client struct { // NewClient returns an authenticated Pagure API client with // the provided configuration. If a nil httpClient is provided, http.DefaultClient // will be used. -func NewClient(urn string, config *schema.PagureConnection, httpClient httpcli.Doer) (*Client, error) { +func NewClient(urn string, config *schema.PagureConnection, httpClient httpcli.Doer, logger log.Logger) (*Client, error) { u, err := url.Parse(config.Url) if err != nil { return nil, err } if httpClient == nil { - httpClient = httpcli.ExternalDoer + httpClient = httpcli.ExternalDoer(logger.Scoped("httpclient")) } return &Client{ diff --git a/internal/extsvc/pagure/client_test.go b/internal/extsvc/pagure/client_test.go index bae6c82596fe5..eedef71bb0128 100644 --- a/internal/extsvc/pagure/client_test.go +++ b/internal/extsvc/pagure/client_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/inconshreveable/log15" //nolint:logging // TODO move all logging to sourcegraph/log + "github.com/sourcegraph/log/logtest" "github.com/sourcegraph/sourcegraph/internal/testutil" ) @@ -14,7 +15,7 @@ import ( var update = flag.Bool("update", false, "update testdata") func TestClient_ListProjects(t *testing.T) { - cli, save := NewTestClient(t, "ListRepos", *update) + cli, save := NewTestClient(t, "ListRepos", *update, logtest.Scoped(t)) defer save() ctx := context.Background() diff --git a/internal/extsvc/pagure/testing.go b/internal/extsvc/pagure/testing.go index ffb24dce98e2b..a1ef0e0b94730 100644 --- a/internal/extsvc/pagure/testing.go +++ b/internal/extsvc/pagure/testing.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/dnaeon/go-vcr/cassette" + "github.com/sourcegraph/log" "golang.org/x/time/rate" "github.com/sourcegraph/sourcegraph/internal/httpcli" @@ -19,7 +20,7 @@ import ( // NewTestClient returns a pagure.Client that records its interactions // to testdata/vcr/. -func NewTestClient(t testing.TB, name string, update bool) (*Client, func()) { +func NewTestClient(t testing.TB, name string, update bool, logger log.Logger) (*Client, func()) { t.Helper() cassete := filepath.Join("testdata/vcr/", normalize(name)) @@ -44,7 +45,7 @@ func NewTestClient(t testing.TB, name string, update bool) (*Client, func()) { Url: instanceURL, } - cli, err := NewClient("urn", c, hc) + cli, err := NewClient("urn", c, hc, logger.Scoped("pagure.testclient")) if err != nil { t.Fatal(err) } diff --git a/internal/httpcli/client.go b/internal/httpcli/client.go index 0f1fef2df8161..e464afd5a538f 100644 --- a/internal/httpcli/client.go +++ b/internal/httpcli/client.go @@ -100,7 +100,9 @@ var CachedTransportOpt = NewCachedTransportOpt(redisCache, true) // WARN: Clients from this factory cache entire responses for etag matching. Do not // use them for one-off requests if possible, and definitely not for larger payloads, // like downloading arbitrarily sized files! See UncachedExternalClientFactory instead. -var ExternalClientFactory = NewExternalClientFactory() +func ExternalClientFactory(logger log.Logger) *Factory { + return NewExternalClientFactory(NewLoggingMiddleware(logger)) +} // UncachedExternalClientFactory is a httpcli.Factory with common options // and middleware pre-set for communicating with external services, but with caching @@ -175,7 +177,10 @@ func newExternalClientFactory(cache bool, testOpt bool, middleware ...Middleware // WARN: This client caches entire responses for etag matching. Do not use it for // one-off requests if possible, and definitely not for larger payloads, like // downloading arbitrarily sized files! See UncachedExternalDoer instead. -var ExternalDoer, _ = ExternalClientFactory.Doer() +func ExternalDoer(logger log.Logger) Doer { + d, _ := ExternalClientFactory(logger).Doer() + return d +} // UncachedExternalDoer is a shared client for external communication. This is a // convenience for existing uses of http.DefaultClient. @@ -204,7 +209,10 @@ var TestExternalDoer, _ = TestExternalClientFactory.Doer() // WARN: This client caches entire responses for etag matching. Do not use it for // one-off requests if possible, and definitely not for larger payloads, like // downloading arbitrarily sized files! See UncachedExternalClient instead. -var ExternalClient, _ = ExternalClientFactory.Client() +func ExternalClient(logger log.Logger) *http.Client { + c, _ := ExternalClientFactory(logger).Client() + return c +} // UncachedExternalClient returns a shared client for external communication. This is // a convenience for existing uses of http.DefaultClient. diff --git a/internal/repos/awscodecommit.go b/internal/repos/awscodecommit.go index abebe242beb07..2a14f7a39d98e 100644 --- a/internal/repos/awscodecommit.go +++ b/internal/repos/awscodecommit.go @@ -10,6 +10,7 @@ import ( "github.com/aws/aws-sdk-go-v2/config" awscredentials "github.com/aws/aws-sdk-go-v2/credentials" "github.com/aws/aws-sdk-go-v2/service/codecommit" + "github.com/sourcegraph/log" "golang.org/x/net/http2" "github.com/sourcegraph/sourcegraph/lib/errors" @@ -37,7 +38,7 @@ type AWSCodeCommitSource struct { } // NewAWSCodeCommitSource returns a new AWSCodeCommitSource from the given external service. -func NewAWSCodeCommitSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory) (*AWSCodeCommitSource, error) { +func NewAWSCodeCommitSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory, logger log.Logger) (*AWSCodeCommitSource, error) { rawConfig, err := svc.Config.Decrypt(ctx) if err != nil { return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err) @@ -46,12 +47,12 @@ func NewAWSCodeCommitSource(ctx context.Context, svc *types.ExternalService, cf if err := jsonc.Unmarshal(rawConfig, &c); err != nil { return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err) } - return newAWSCodeCommitSource(svc, &c, cf) + return newAWSCodeCommitSource(svc, &c, cf, logger) } -func newAWSCodeCommitSource(svc *types.ExternalService, c *schema.AWSCodeCommitConnection, cf *httpcli.Factory) (*AWSCodeCommitSource, error) { +func newAWSCodeCommitSource(svc *types.ExternalService, c *schema.AWSCodeCommitConnection, cf *httpcli.Factory, logger log.Logger) (*AWSCodeCommitSource, error) { if cf == nil { - cf = httpcli.ExternalClientFactory + cf = httpcli.ExternalClientFactory(logger.Scoped("awscodecommit")) } cli, err := cf.Doer(func(c *http.Client) error { diff --git a/internal/repos/awscodecommit_test.go b/internal/repos/awscodecommit_test.go index 79e22e503c447..88fe243374e7f 100644 --- a/internal/repos/awscodecommit_test.go +++ b/internal/repos/awscodecommit_test.go @@ -3,6 +3,8 @@ package repos import ( "testing" + "github.com/sourcegraph/log/logtest" + "github.com/sourcegraph/sourcegraph/internal/extsvc" "github.com/sourcegraph/sourcegraph/internal/extsvc/awscodecommit" "github.com/sourcegraph/sourcegraph/internal/httpcli" @@ -24,7 +26,7 @@ func TestAWSCodeCommitSource_Exclude(t *testing.T) { fact := httpcli.NewFactory(httpcli.NewMiddleware()) svc := types.ExternalService{Kind: extsvc.KindAWSCodeCommit, Config: extsvc.NewEmptyConfig()} - conn, err := newAWSCodeCommitSource(&svc, config, fact) + conn, err := newAWSCodeCommitSource(&svc, config, fact, logtest.Scoped(t)) if err != nil { t.Fatal(err) } diff --git a/internal/repos/azuredevops.go b/internal/repos/azuredevops.go index b3b9d8b18c79a..edde79482c686 100644 --- a/internal/repos/azuredevops.go +++ b/internal/repos/azuredevops.go @@ -32,6 +32,7 @@ type AzureDevOpsSource struct { // NewAzureDevOpsSource returns a new AzureDevOpsSource from the given external service. func NewAzureDevOpsSource(ctx context.Context, logger log.Logger, svc *types.ExternalService, cf *httpcli.Factory) (*AzureDevOpsSource, error) { + logger = logger.Scoped("AzureDevOpsSource") rawConfig, err := svc.Config.Decrypt(ctx) if err != nil { return nil, errors.Wrapf(err, "external service id=%d config", svc.ID) @@ -42,7 +43,7 @@ func NewAzureDevOpsSource(ctx context.Context, logger log.Logger, svc *types.Ext } if cf == nil { - cf = httpcli.ExternalClientFactory + cf = httpcli.ExternalClientFactory(logger) } httpCli, err := cf.Doer() @@ -50,7 +51,7 @@ func NewAzureDevOpsSource(ctx context.Context, logger log.Logger, svc *types.Ext return nil, err } - cli, err := azuredevops.NewClient(svc.URN(), c.Url, &auth.BasicAuth{Username: c.Username, Password: c.Token}, httpCli) + cli, err := azuredevops.NewClient(svc.URN(), c.Url, &auth.BasicAuth{Username: c.Username, Password: c.Token}, httpCli, logger) if err != nil { return nil, err } diff --git a/internal/repos/azuredevops_test.go b/internal/repos/azuredevops_test.go index 06c9bec148462..89f382faaa12e 100644 --- a/internal/repos/azuredevops_test.go +++ b/internal/repos/azuredevops_test.go @@ -5,6 +5,8 @@ import ( "os" "testing" + "github.com/sourcegraph/log/logtest" + "github.com/sourcegraph/sourcegraph/internal/extsvc" "github.com/sourcegraph/sourcegraph/internal/ratelimit" "github.com/sourcegraph/sourcegraph/internal/testutil" @@ -38,7 +40,7 @@ func TestAzureDevOpsSource_ListRepos(t *testing.T) { }) ctx := context.Background() - src, err := NewAzureDevOpsSource(ctx, nil, svc, cf) + src, err := NewAzureDevOpsSource(ctx, logtest.Scoped(t), svc, cf) if err != nil { t.Fatal(err) } diff --git a/internal/repos/bitbucketcloud.go b/internal/repos/bitbucketcloud.go index 53e27f2fdfb2d..d0207998f870e 100644 --- a/internal/repos/bitbucketcloud.go +++ b/internal/repos/bitbucketcloud.go @@ -48,8 +48,9 @@ func NewBitbucketCloudSource(ctx context.Context, logger log.Logger, svc *types. } func newBitbucketCloudSource(logger log.Logger, svc *types.ExternalService, c *schema.BitbucketCloudConnection, cf *httpcli.Factory) (*BitbucketCloudSource, error) { + logger = logger.Scoped("BitbucketCloudSource") if cf == nil { - cf = httpcli.ExternalClientFactory + cf = httpcli.ExternalClientFactory(logger) } cli, err := cf.Doer() @@ -69,7 +70,7 @@ func newBitbucketCloudSource(logger log.Logger, svc *types.ExternalService, c *s return nil, err } - client, err := bitbucketcloud.NewClient(svc.URN(), c, cli) + client, err := bitbucketcloud.NewClient(svc.URN(), c, cli, logger) if err != nil { return nil, err } diff --git a/internal/repos/bitbucketserver.go b/internal/repos/bitbucketserver.go index 716ce1badc355..1622ba36810c2 100644 --- a/internal/repos/bitbucketserver.go +++ b/internal/repos/bitbucketserver.go @@ -50,8 +50,9 @@ func NewBitbucketServerSource(ctx context.Context, logger log.Logger, svc *types } func newBitbucketServerSource(logger log.Logger, svc *types.ExternalService, c *schema.BitbucketServerConnection, cf *httpcli.Factory) (*BitbucketServerSource, error) { + logger = logger.Scoped("BitbucketServerSource") if cf == nil { - cf = httpcli.ExternalClientFactory + cf = httpcli.ExternalClientFactory(logger) } var opts []httpcli.Opt @@ -80,7 +81,7 @@ func newBitbucketServerSource(logger log.Logger, svc *types.ExternalService, c * return nil, err } - client, err := bitbucketserver.NewClient(svc.URN(), c, cli) + client, err := bitbucketserver.NewClient(svc.URN(), c, cli, logger) if err != nil { return nil, err } diff --git a/internal/repos/bitbucketserver_test.go b/internal/repos/bitbucketserver_test.go index 3c04311dec99c..d35775fe849fb 100644 --- a/internal/repos/bitbucketserver_test.go +++ b/internal/repos/bitbucketserver_test.go @@ -472,7 +472,7 @@ func TestBitbucketServerSource_ListByProjectKeyAuthentic(t *testing.T) { if err != nil { t.Fatal(err) } - cli := bitbucketserver.NewTestClient(t, name, Update(name)) + cli := bitbucketserver.NewTestClient(t, logtest.Scoped(t), name, Update(name)) s.client = cli // This project has 2 repositories in it. that's why we expect 2 diff --git a/internal/repos/gerrit.go b/internal/repos/gerrit.go index 06ac504912905..2fd0cdbfb7620 100644 --- a/internal/repos/gerrit.go +++ b/internal/repos/gerrit.go @@ -8,6 +8,8 @@ import ( "strconv" "strings" + "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/conf/reposource" "github.com/sourcegraph/sourcegraph/internal/extsvc" @@ -38,7 +40,7 @@ type GerritSource struct { } // NewGerritSource returns a new GerritSource from the given external service. -func NewGerritSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory) (*GerritSource, error) { +func NewGerritSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory, logger log.Logger) (*GerritSource, error) { rawConfig, err := svc.Config.Decrypt(ctx) if err != nil { return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err) @@ -48,8 +50,10 @@ func NewGerritSource(ctx context.Context, svc *types.ExternalService, cf *httpcl return nil, errors.Wrapf(err, "external service id=%d config error", svc.ID) } + logger = logger.Scoped("GerritSource") + if cf == nil { - cf = httpcli.ExternalClientFactory + cf = httpcli.ExternalClientFactory(logger) } httpCli, err := cf.Doer() @@ -65,7 +69,7 @@ func NewGerritSource(ctx context.Context, svc *types.ExternalService, cf *httpcl cli, err := gerrit.NewClient(svc.URN(), u, &gerrit.AccountCredentials{ Username: c.Username, Password: c.Password, - }, httpCli) + }, httpCli, logger) if err != nil { return nil, err } diff --git a/internal/repos/gerrit_test.go b/internal/repos/gerrit_test.go index 2d8ee9bb6ba6a..b29eb1ebd9baa 100644 --- a/internal/repos/gerrit_test.go +++ b/internal/repos/gerrit_test.go @@ -5,6 +5,7 @@ import ( "os" "testing" + "github.com/sourcegraph/log/logtest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -29,7 +30,7 @@ func TestGerritSource_ListRepos(t *testing.T) { }) ctx := context.Background() - src, err := NewGerritSource(ctx, svc, cf) + src, err := NewGerritSource(ctx, svc, cf, logtest.Scoped(t)) require.NoError(t, err) src.perPage = 25 @@ -52,7 +53,7 @@ func TestGerritSource_ListRepos(t *testing.T) { }) ctx := context.Background() - src, err := NewGerritSource(ctx, svc, cf) + src, err := NewGerritSource(ctx, svc, cf, logtest.Scoped(t)) require.NoError(t, err) repos, err := ListAll(ctx, src) @@ -75,7 +76,7 @@ func TestGerritSource_ListRepos(t *testing.T) { }) ctx := context.Background() - src, err := NewGerritSource(ctx, svc, cf) + src, err := NewGerritSource(ctx, svc, cf, logtest.Scoped(t)) require.NoError(t, err) src.perPage = 25 @@ -105,7 +106,7 @@ func TestGerritSource_ListRepos(t *testing.T) { }) ctx := context.Background() - src, err := NewGerritSource(ctx, svc, cf) + src, err := NewGerritSource(ctx, svc, cf, logtest.Scoped(t)) require.NoError(t, err) src.perPage = 25 @@ -133,7 +134,7 @@ func TestGerritSource_ListRepos(t *testing.T) { }) ctx := context.Background() - src, err := NewGerritSource(ctx, svc, cf) + src, err := NewGerritSource(ctx, svc, cf, logtest.Scoped(t)) require.NoError(t, err) src.perPage = 25 @@ -156,7 +157,7 @@ func TestGerritSource_ListRepos(t *testing.T) { }) ctx := context.Background() - src, err := NewGerritSource(ctx, svc, cf) + src, err := NewGerritSource(ctx, svc, cf, logtest.Scoped(t)) require.NoError(t, err) src.perPage = 25 diff --git a/internal/repos/github.go b/internal/repos/github.go index 593593ec0ef99..9deea463504ab 100644 --- a/internal/repos/github.go +++ b/internal/repos/github.go @@ -93,6 +93,8 @@ func newGitHubSource( c *schema.GitHubConnection, cf *httpcli.Factory, ) (*GitHubSource, error) { + logger = logger.Scoped("GitHubSource") + baseURL, err := url.Parse(c.Url) if err != nil { return nil, err @@ -103,7 +105,7 @@ func newGitHubSource( apiURL, githubDotCom := github.APIRoot(baseURL) if cf == nil { - cf = httpcli.ExternalClientFactory + cf = httpcli.ExternalClientFactory(logger) } opts := []httpcli.Opt{ @@ -177,11 +179,11 @@ func newGitHubSource( urn := svc.URN() var ( - v3ClientLogger = log.Scoped("source") + v3ClientLogger = logger.Scoped("source") v3Client = github.NewV3Client(v3ClientLogger, urn, apiURL, auther, cli) - v4Client = github.NewV4Client(urn, apiURL, auther, cli) + v4Client = github.NewV4Client(urn, apiURL, auther, cli, logger.Scoped("v4client")) - searchClientLogger = log.Scoped("search") + searchClientLogger = logger.Scoped("search") searchClient = github.NewV3SearchClient(searchClientLogger, urn, apiURL, auther, cli) ) diff --git a/internal/repos/github_test.go b/internal/repos/github_test.go index 2154ac4dbb3d4..514ae8330f207 100644 --- a/internal/repos/github_test.go +++ b/internal/repos/github_test.go @@ -603,7 +603,7 @@ func TestGitHubSource_doRecursively(t *testing.T) { apiURL, err := url.Parse(srv.URL) require.NoError(t, err) - ghCli := github.NewV4Client("", apiURL, nil, httpcli.TestExternalDoer) + ghCli := github.NewV4Client("", apiURL, nil, httpcli.TestExternalDoer, logtest.Scoped(t)) q := newRepositoryQuery("stars:>=5", ghCli, logtest.NoOp(t)) q.Limit = 5 @@ -969,7 +969,7 @@ func TestRepositoryQuery_DoWithRefinedWindow(t *testing.T) { Query: tc.query, First: tc.first, Limit: tc.limit, - Searcher: github.NewV4Client("Test", apiURL, token, cli), + Searcher: github.NewV4Client("Test", apiURL, token, cli, logtest.Scoped(t)), } results := make(chan *githubResult) @@ -1041,7 +1041,7 @@ func TestRepositoryQuery_DoSingleRequest(t *testing.T) { Query: tc.query, First: tc.first, Limit: tc.limit, - Searcher: github.NewV4Client("Test", apiURL, token, cli), + Searcher: github.NewV4Client("Test", apiURL, token, cli, logtest.Scoped(t)), } results := make(chan *githubResult) diff --git a/internal/repos/gitlab.go b/internal/repos/gitlab.go index 99dd41249215d..931d10ed4222e 100644 --- a/internal/repos/gitlab.go +++ b/internal/repos/gitlab.go @@ -69,6 +69,8 @@ var gitlabRatelimitWaitCounter = promauto.NewCounterVec(prometheus.CounterOpts{ }, []string{"resource", "name"}) func newGitLabSource(logger log.Logger, svc *types.ExternalService, c *schema.GitLabConnection, cf *httpcli.Factory) (*GitLabSource, error) { + logger = logger.Scoped("GitLabSource") + baseURL, err := url.Parse(c.Url) if err != nil { return nil, err @@ -76,7 +78,7 @@ func newGitLabSource(logger log.Logger, svc *types.ExternalService, c *schema.Gi baseURL = extsvc.NormalizeBaseURL(baseURL) if cf == nil { - cf = httpcli.ExternalClientFactory + cf = httpcli.ExternalClientFactory(logger) } var opts []httpcli.Opt @@ -120,7 +122,7 @@ func newGitLabSource(logger log.Logger, svc *types.ExternalService, c *schema.Gi return nil, err } - provider := gitlab.NewClientProvider(svc.URN(), baseURL, cli) + provider := gitlab.NewClientProvider(svc.URN(), baseURL, cli, logger) var client *gitlab.Client switch gitlab.TokenType(c.TokenType) { diff --git a/internal/repos/gitlab_test.go b/internal/repos/gitlab_test.go index 6aa0002fae405..cda7d5531f315 100644 --- a/internal/repos/gitlab_test.go +++ b/internal/repos/gitlab_test.go @@ -312,7 +312,7 @@ func TestGitlabSource_ListRepos(t *testing.T) { svc := typestest.MakeExternalService(t, extsvc.VariantGitLab, conf) ctx := context.Background() - src, err := NewGitLabSource(ctx, nil, svc, cf) + src, err := NewGitLabSource(ctx, logtest.Scoped(t), svc, cf) if err != nil { t.Fatal(err) } diff --git a/internal/repos/other.go b/internal/repos/other.go index 4e97a40f4696a..d625d6bc3b2ef 100644 --- a/internal/repos/other.go +++ b/internal/repos/other.go @@ -43,6 +43,8 @@ type ( // NewOtherSource returns a new OtherSource from the given external service. func NewOtherSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory, logger log.Logger) (*OtherSource, error) { + logger = logger.Scoped("OtherSource") + rawConfig, err := svc.Config.Decrypt(ctx) if err != nil { return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err) @@ -53,7 +55,7 @@ func NewOtherSource(ctx context.Context, svc *types.ExternalService, cf *httpcli } if cf == nil { - cf = httpcli.ExternalClientFactory + cf = httpcli.ExternalClientFactory(logger) } cli, err := cf.Doer() diff --git a/internal/repos/other_test.go b/internal/repos/other_test.go index b4353406da593..16909180427f1 100644 --- a/internal/repos/other_test.go +++ b/internal/repos/other_test.go @@ -159,7 +159,7 @@ func TestSrcExpose_SrcExposeServer(t *testing.T) { ID: 1, Kind: extsvc.KindOther, Config: extsvc.NewUnencryptedConfig(fmt.Sprintf(`{"url": %q, "repos": ["%s"]}`, s.URL, "src-expose")), - }, httpcli.TestExternalClientFactory, nil) + }, httpcli.TestExternalClientFactory, logtest.Scoped(t)) if err != nil { t.Fatal(err) } @@ -188,7 +188,7 @@ func TestOther_DotComConfig(t *testing.T) { ID: 1, Kind: extsvc.KindOther, Config: extsvc.NewUnencryptedConfig(fmt.Sprintf(`{"url": "somegit.com/repo", "repos": ["%s"], "makeReposPublicOnDotCom": true}`, "src-expose")), - }, nil, nil) + }, nil, logtest.Scoped(t)) require.NoError(t, err) return source } diff --git a/internal/repos/pagure.go b/internal/repos/pagure.go index 4e225e53efa36..5e75d1fe8d7ce 100644 --- a/internal/repos/pagure.go +++ b/internal/repos/pagure.go @@ -6,6 +6,7 @@ import ( "strconv" "github.com/goware/urlx" + "github.com/sourcegraph/log" "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/extsvc" @@ -27,7 +28,7 @@ type PagureSource struct { } // NewPagureSource returns a new PagureSource from the given external service. -func NewPagureSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory) (*PagureSource, error) { +func NewPagureSource(ctx context.Context, svc *types.ExternalService, cf *httpcli.Factory, logger log.Logger) (*PagureSource, error) { rawConfig, err := svc.Config.Decrypt(ctx) if err != nil { return nil, errors.Errorf("external service id=%d config error: %s", svc.ID, err) @@ -38,7 +39,7 @@ func NewPagureSource(ctx context.Context, svc *types.ExternalService, cf *httpcl } if cf == nil { - cf = httpcli.ExternalClientFactory + cf = httpcli.ExternalClientFactory(logger.Scoped("pagure.source.httpclient")) } httpCli, err := cf.Doer() @@ -46,7 +47,7 @@ func NewPagureSource(ctx context.Context, svc *types.ExternalService, cf *httpcl return nil, err } - cli, err := pagure.NewClient(svc.URN(), &c, httpCli) + cli, err := pagure.NewClient(svc.URN(), &c, httpCli, logger.Scoped("pagure.httpclient")) if err != nil { return nil, err } diff --git a/internal/repos/pagure_test.go b/internal/repos/pagure_test.go index fed663aa41907..672194bf7b2b7 100644 --- a/internal/repos/pagure_test.go +++ b/internal/repos/pagure_test.go @@ -4,6 +4,8 @@ import ( "context" "testing" + "github.com/sourcegraph/log/logtest" + "github.com/sourcegraph/sourcegraph/internal/testutil" "github.com/sourcegraph/sourcegraph/internal/types/typestest" @@ -22,7 +24,7 @@ func TestPagureSource_ListRepos(t *testing.T) { svc := typestest.MakeExternalService(t, extsvc.VariantPagure, conf) ctx := context.Background() - src, err := NewPagureSource(ctx, svc, cf) + src, err := NewPagureSource(ctx, svc, cf, logtest.Scoped(t)) if err != nil { t.Fatal(err) } diff --git a/internal/repos/sources.go b/internal/repos/sources.go index bb3c1237718b9..577643247d0fc 100644 --- a/internal/repos/sources.go +++ b/internal/repos/sources.go @@ -43,6 +43,7 @@ func NewSourcer(logger log.Logger, db database.DB, cf *httpcli.Factory, gc gitse // NewSource returns a repository yielding Source from the given ExternalService configuration. func NewSource(ctx context.Context, logger log.Logger, db database.DB, svc *types.ExternalService, cf *httpcli.Factory, gc gitserver.Client) (Source, error) { + if gc == nil { gc = gitserver.NewClient("repos.sourcer") } @@ -54,7 +55,7 @@ func NewSource(ctx context.Context, logger log.Logger, db database.DB, svc *type case extsvc.KindAzureDevOps: return NewAzureDevOpsSource(ctx, logger.Scoped("AzureDevOpsSource"), svc, cf) case extsvc.KindGerrit: - return NewGerritSource(ctx, svc, cf) + return NewGerritSource(ctx, svc, cf, logger.Scoped("GerritSource")) case extsvc.KindBitbucketServer: return NewBitbucketServerSource(ctx, logger.Scoped("BitbucketServerSource"), svc, cf) case extsvc.KindBitbucketCloud: @@ -64,7 +65,7 @@ func NewSource(ctx context.Context, logger log.Logger, db database.DB, svc *type case extsvc.KindPhabricator: return NewPhabricatorSource(ctx, logger.Scoped("PhabricatorSource"), svc, cf) case extsvc.KindAWSCodeCommit: - return NewAWSCodeCommitSource(ctx, svc, cf) + return NewAWSCodeCommitSource(ctx, svc, cf, logger.Scoped("AWSCodeCommitSource")) case extsvc.KindPerforce: return NewPerforceSource(ctx, svc) case extsvc.KindGoPackages: @@ -73,7 +74,7 @@ func NewSource(ctx context.Context, logger log.Logger, db database.DB, svc *type // JVM doesn't need a client factory because we use coursier. return NewJVMPackagesSource(ctx, svc) case extsvc.KindPagure: - return NewPagureSource(ctx, svc, cf) + return NewPagureSource(ctx, svc, cf, logger.Scoped("PagureSource")) case extsvc.KindNpmPackages: return NewNpmPackagesSource(ctx, svc, cf) case extsvc.KindPythonPackages: diff --git a/internal/updatecheck/client.go b/internal/updatecheck/client.go index f95cd4a0e191c..1dda669add848 100644 --- a/internal/updatecheck/client.go +++ b/internal/updatecheck/client.go @@ -856,7 +856,7 @@ func check(logger log.Logger, db database.DB) { var doer httpcli.Doer if telemetryHTTPProxy == "" { - doer = httpcli.ExternalDoer + doer = httpcli.ExternalDoer(logger.Scoped("UpdateCheck")) } else { u, err := url.Parse(telemetryHTTPProxy) if err != nil { diff --git a/internal/updatecheck/handler.go b/internal/updatecheck/handler.go index 913c6a11c41fd..d16c9bd2c2a6a 100644 --- a/internal/updatecheck/handler.go +++ b/internal/updatecheck/handler.go @@ -393,7 +393,7 @@ func logPing(logger log.Logger, pubsubClient pubsub.TopicPublisher, meter *Meter now := time.Now().UTC() rounded := time.Date(now.Year(), now.Month(), now.Day(), 0, 0, 0, 0, now.Location()) millis := rounded.UnixNano() / (int64(time.Millisecond) / int64(time.Nanosecond)) - go hubspotutil.SyncUser(pr.InitialAdminEmail, "", &hubspot.ContactProperties{IsServerAdmin: true, LatestPing: millis, HasAgreedToToS: pr.TosAccepted}) + go hubspotutil.SyncUser(logger, pr.InitialAdminEmail, "", &hubspot.ContactProperties{IsServerAdmin: true, LatestPing: millis, HasAgreedToToS: pr.TosAccepted}) } var clientAddr string