Expose Client.Client #2015
Expose Client.Client #2015
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2015 +/- ##
=======================================
Coverage 97.86% 97.86%
=======================================
Files 107 107
Lines 6897 6897
=======================================
Hits 6750 6750
Misses 81 81
Partials 66 66
Continue to review full report at Codecov.
|
| @@ -137,7 +137,7 @@ var errNonNilContext = errors.New("context must be non-nil") | |||
| // A Client manages communication with the GitHub API. | |||
| type Client struct { | |||
| clientMu sync.Mutex // clientMu protects the client during calls that modify the CheckRedirect func. | |||
There was a problem hiding this comment.
Will exposing client directly lead to problems with this mutex? I think two different github.Client objects would each use separate locks to protect the same *http.Client object, leading to race conditions.
Maybe instead there could be a new Client() or HTTPClient() function that returns a shallow copy of the internal client field? That would enable reuse of the transport and timeout while allowing each instance to safely update the redirect function.
There was a problem hiding this comment.
Hmmm... that's an interesting question...
The idea was to prevent multiple calls that check for redirects to avoid swapping out the client at the same time.
But maybe a read-only Client() method as you recommend would be a better solution anyway.
|
Tested this with my real-world code that could use it (below). Works great, I'm not using any concurrency. // Check performs the policy check for Binary Artifacts based on the
// configuration stored in the org/repo, implementing policydef.Policy.Check()
func (b Binary) Check(ctx context.Context, c *github.Client, owner,
repo string) (*policydef.Result, error) {
oc, rc := getConfig(ctx, c, owner, repo)
enabled := config.IsEnabled(oc.OptConfig, rc.OptConfig, repo)
log.Info().
Str("org", owner).
Str("repo", repo).
Str("area", polName).
Bool("enabled", enabled).
Msg("Check repo enabled")
gh32c := gh32.NewClient(c.Client)
repoClient := githubrepo.CreateGithubRepoClient(ctx, gh32c)
if err := repoClient.InitRepo(owner, repo); err != nil {
return nil, err
}
l := logger{}
cr := &checker.CheckRequest{
Ctx: ctx,
Client: gh32c,
RepoClient: repoClient,
HTTPClient: nil,
Owner: owner,
Repo: repo,
GraphClient: nil,
Logf: l.Logf,
}
// TODO, likely this should be a "scorecard" policy that runs multiple checks
// here, and uses config to enable/disable checks.
res := checks.BinaryArtifacts(cr)
if res.Error != nil {
return nil, res.Error
} |
|
Thank you, @jeffmendoza - hey, how about if I make a read-only version of the Client as described by @bluekeyes above ? |
|
I don't see any reason why that wouldn't work as well. |
|
Closing due to preferred solution in #2016. |
Fixes: #1859.