Add the ability to mock more SCM data for git resolver tests #5531
Conversation
tekton-robot
added
the
release-note-none
tekton-robot
added
the
size/M
|
/assign @chuangw6 @lbernick @vdemeester |
|
@abayer: GitHub didn't allow me to assign the following users: chuangw6. Note that only tektoncd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chuangw6, vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
| @@ -147,19 +152,32 @@ func (r *Resolver) resolveAPIGit(ctx context.Context, params map[string]string) | |||
| if err != nil { | |||
| return nil, fmt.Errorf("failed to create SCM client: %w", err) | |||
| } | |||
|
|
|||
| // If we have fake data for the scm client, replace the scm client with a fake client with the fake data | |||
| if r.fakeData != nil && scmClient.Driver == scm.DriverFake { | |||
There was a problem hiding this comment.
would it make more sense to pass a client in as an argument to a helper function, or store the client on the resolver or something like that? (i.e. more like the dependency injection we use elsewhere in the codebase). It feels strange to mix test functionality and real functionality in the resolver code.
There was a problem hiding this comment.
I made the deliberate choice to create the client at resolution time every time. That way we don't have to specifically watch for changes to the token secret and then create a new client, and the work I'm doing for #5487 will mean we support multiple providers, which means different clients will be needed at runtime for each client, etc.
I guess we could have a field on the resolver that's a function for creating the client, with the default implementation being what we do now, and tests can override that?
There was a problem hiding this comment.
maybe the resolution function can accept a client creation function as an argument? kind of like getpipelinefunc
There was a problem hiding this comment.
It would need to be on the resolver, not the function, since the actual path to calling Resolve(...) in the tests is off in frtesting.RunResolverReconcileTest. Lemme try something.
tekton-robot
added
the
do-not-merge/hold
|
I like that approach! |
|
The following is the coverage report on the affected files.
|
|
woo! Squashing it down now. |
Specifically, it adds the ability to cheat a "fake" list of repositories into the SCM client for use in adding the clone URL to the resolved resource annotations, as needed in tektoncd#5397. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
abayer
force-pushed
the
scm-fake-data
branch
from
69171b7 to
9b83c33
Compare
|
/hold cancel |
tekton-robot
removed
the
do-not-merge/hold
|
The following is the coverage report on the affected files.
|
|
/lgtm |
Changes
Specifically, it adds the ability to cheat a "fake" list of repositories into the SCM client for use in adding the clone URL to the resolved resource annotations, as needed in #5397.
/kind misc
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes