subversion-plugin icon indicating copy to clipboard operation
subversion-plugin copied to clipboard

[JENKINS-35227] added support for AdditionalCredentials to SubversionSCMSource

Open r-funke opened this issue 7 years ago • 10 comments

This fixes issues with multibranch pipeline jobs and SVN externals. The main problem was that SubversionSCMSource did not allow to configure additional credentials, which caused "svn: E200015: ISVNAuthentication" errors. This patch fixes that. The second problem was that during "Scan Multibranch Pipeline" it was just checked whether the revision number of the SVN root directory changed. Thus it did not detect changes in externals. That was fixed by declaring revision number non-deterministic in SCMRevisionImpl, if additional credentials are configured. Then, Jenkins polls for changes in the whole repository.

This should fix JENKINS-35227 and JENKINS-32167. I am not able to post a comment in the bug reports, because I failed to create an account on jenkins.io.

r-funke avatar Jun 20 '17 14:06 r-funke

@jglick thank you for your input. I've moved AdditionalCredentials back in place.

Regarding the non-deterministic revision, it was an easy solution (or workaround) for Jenkins not detecting changes in externals. Of course, the clean solution would be to extend SCMRevisionImpl. But to me it seems that it requires quite a lot of effort to get that done.

Would it be okay to have a checkbox in the config and let the user specify whether isDeterministic() should return true or false?

r-funke avatar Jun 22 '17 11:06 r-funke

Would it help to remove the changes to SCMRevisionImpl and isDeterministic() from this PR to get it accepted? Then this PR would only cover AdditionalCredentials for SubversionSCMSource, i.e. for multibranch pipeline jobs, which would solve JENKINS-35227.

The reason why I changed SCMRevisionImpl is, because "Scan multibranch pipeline" is not working properly with externals. But as far as I see it, there is no JIRA issue for that problem yet. So maybe I should create an issue for that first.

r-funke avatar Jun 27 '17 13:06 r-funke

So maybe I should create an issue for that first.

+1. An issue would be useful. The scope of this change is not that big, so probably we could have two fixes in a single PR

oleg-nenashev avatar Jul 04 '17 10:07 oleg-nenashev

Separate pull requests would be better though

oleg-nenashev avatar Jul 04 '17 10:07 oleg-nenashev

Okay, this PR is now only about additional credentials. Is it good enough to be accepted?

r-funke avatar Jul 07 '17 13:07 r-funke

Yes, it looks much better now. I probably could live even without tests for it. @jglick WDYT?

oleg-nenashev avatar Jul 07 '17 14:07 oleg-nenashev

Any news on this issue?

r-funke avatar Jul 24 '17 10:07 r-funke

Any news on this issue? This fix would help me.

DavidA2014 avatar Sep 04 '17 10:09 DavidA2014

Still would benefit from tests. Probably ought to be migrated to traits in a newer scm-api. Requires a maintainer to do this work I think.

jglick avatar Sep 13 '17 16:09 jglick

When this will be released?

imakowski avatar Mar 04 '18 20:03 imakowski