jira: general client improvements #26564
Conversation
k8s-ci-robot
added
cncf-cla: yes
This PR fixes the fakejira `CloneIssue` and `CreateIssue` functions along with adding unit tests for them, adds more extensive issue function errors for the fakeclient, and modifies the `DeleteRemoteLinkViaURL` function to return a `bool` indicating whether a link with the provided URL was found and removed (no remote link with the provided URL is not an error). These improvements will be used in a new openshift external prow plugin called the `jira-lifecycle` plugin, which will replicate the `bugzilla` plugins functionality, but with the Jira service instead of bugzilla.
AlexNPavel
force-pushed
the
jira-client-improvements
branch
from
6e96f94 to
4534311
Compare
k8s-ci-robot
removed
the
approved
| f.updateIssueIDAndKey(issueCopy) | ||
| // run generic cloning function | ||
| return jiraclient.CloneIssue(f, issueCopy) | ||
| return jiraclient.CloneIssue(f, issue) | ||
| } | ||
|
|
||
| func (f *FakeClient) CreateIssue(issue *jira.Issue) (*jira.Issue, error) { |
There was a problem hiding this comment.
What, exactly, is this helping? Having to build a fake Jira sounds like testing is happening at the wrong level. See also: how this repo has not built a fake GitHub, Bugzilla, S3, etc.
There was a problem hiding this comment.
What do you mean by this repo not building a fake github or bugzilla? There is a fakegithub and a fake bugzilla client that are used heavily by unit testing in plugins. This fake jira client isn't much different.
There was a problem hiding this comment.
Those are simple in-memory maps that we add to / delete from using the methods. They do not encode provider-specific implementation details like ("issue.fields.project must be set to create new issue").
prow/jira/fakejira/fake_test.go
Outdated
| ptrIssues = append(ptrIssues, &tc.existingIssues[index]) | ||
| } | ||
| fakeClient := &FakeClient{Issues: ptrIssues} | ||
| newIssue, err := fakeClient.CreateIssue(&tc.issue) |
There was a problem hiding this comment.
These are unit tests for a fake ... ? This doesn't seem right - in the fake you might inject data, or validate that calls happen to the right endpoints. There's nothing making sure that the fake implementation of the clone here is the same as what Jira actually does, so ensuring it is "correct" here is not helpful.
There was a problem hiding this comment.
The CreateIssue function was very poorly implemented before to the point that it was basically useless (that was my fault as I wrote it). I just wanted to make sure it was generally close enough when fixing the implementation. For the CloneIssue function, we just call a generic function that is shared by the fake and real clients so this tests that, but makes it does make calls to CreateIssueLink and CreateIssue of the client provided (in this case fake client), so you could say it's not a truly accurate test.
If you would rather not have unit tests for the fake client, I could remove these tests.
There was a problem hiding this comment.
I have removed the unit tests.
AlexNPavel
force-pushed
the
jira-client-improvements
branch
2 times, most recently
from
7878953 to
7d42023
Compare
This commit removes the unit tests for the fake client and simplifies the `CreateIssue` function by removing checks for unsettable fields.
AlexNPavel
force-pushed
the
jira-client-improvements
branch
from
7d42023 to
0587179
Compare
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlexNPavel, stevekuznetsov 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 |
This PR fixes the fakejira
CloneIssueandCreateIssuefunctionsalong with adding unit tests for them, adds more extensive issue
function errors for the fakeclient, and modifies the
DeleteRemoteLinkViaURLfunction to return aboolindicating whethera link with the provided URL was found and removed (no remote link with
the provided URL is not an error).
These improvements will be used in a new openshift external prow plugin
called the
jira-lifecycleplugin, which will replicate thebugzillaplugins functionality, but with the Jira service instead of bugzilla.
/cc @stevekuznetsov @alvaroaleman