[JENKINS-64803] Fix tags retrieval with folder scoped credentials #1139
Conversation
|
First commit showcase the failure. Will commit the fix then to confirm that it fixes it. |
pom.xml
Outdated
| @@ -156,6 +156,16 @@ | |||
| <artifactId>mockito-core</artifactId> | |||
| <scope>test</scope> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.powermock</groupId> | |||
There was a problem hiding this comment.
Thanks very much for contributing tests that show the issue and a fix for the issue.
Based on the comment from @timja in jenkinsci/jenkins#5736 (powermock does not seem to work with Java 17 and has much less frequent releases than mockito), would you consider switching to use the mockito framework that is already used in the git plugin, rather than introducing the powermock framework?
I'm not skilled in mocking. I already struggle to benefit from the mockito based tests and fear that adding another framework will make it more difficult to maintain the plugin.
I removed the powermock framework from the plugin in 2019 with a56d37e It was used by only one test and was making JDK 11 work more difficult.
There was a problem hiding this comment.
Got it. I think I am able to make it work with just Mockito. But I need to bring in mockito-inline.
| SCMRevision rev = source.fetch("lightweight", listener, p); | ||
| assertThat(rev, notNullValue()); | ||
| assertThat(rev.getHead().toString(), equalTo("SCMHead{'lightweight'}")); | ||
| Mockito.verify(gitClient, Mockito.times(0)).addDefaultCredentials(null); |
There was a problem hiding this comment.
Personally I would static import mockito and argument matchers as it leads to easier reading of the test imo, also given doReturn is static imported but the rest aren’t…
Prefer no trailing white space in new code
|
Fixed the initialization of TFS tests that seemed to be impacted. |
|
Hmm.. as soon as a dep on |
src/test/java/hudson/plugins/git/browser/TFS2013GitRepositoryBrowserTest.java
Outdated
Show resolved
Hide resolved
| Collections.singletonList(new UserRemoteConfig(repoUrl, null, null, null)), | ||
| new ArrayList<>(), | ||
| null, JGitTool.MAGIC_EXENAME, | ||
| Collections.<GitSCMExtension>emptyList()); | ||
|
|
There was a problem hiding this comment.
Generally ask that changes of pure white space not be made because there is a large backlog of closed but potentially useful pull requests that will have merge conflicts if white space is adjusted. Yes, the white space is ugly, but it needs to remain ugly until we have agreement that the source code formatting can be maintained by a program.
There was a problem hiding this comment.
Allright. Let me add a commit to preserve it then.
Dohbedoh
changed the title
Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>
JENKINS-64803 - Test folder scoped credentials
In the context of Shared Library, the fetching of tags fails because the implementation of SCMRevision retrieve(@NonNull String thingName, @NonNull TaskListener listener, @CheckForNull Item context) eventually loses the Item context. In such cases, the credentials lookup uses getOwner() that in the case of a pipeline library is
null. Therefore if the credentials provided are defined at a folder level, the lookup does not find them.The suggested fix pass the
retrieveContextpassed from the SCM API does thedoRetrievemethods when we have it.Checklist
Types of changes