[JENKINS-35227] added support for AdditionalCredentials to SubversionSCMSource #189

r-funke · 2017-06-20T14:11:53Z

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.

issues with externals - moved AddedCredentials to separate file (was nested class before) - issues with ISVNAuthentication and externals in multibranch jobs should be fixed now, when specifying additional credentials

jglick · 2017-06-21T18:38:56Z

src/main/java/hudson/scm/SubversionSCM.java

@@ -3238,98 +3254,5 @@ private static StandardCredentials lookupCredentials(Item context, String creden
                        CredentialsMatchers.withId(credentialsId));
    }

-    public static class AdditionalCredentials extends AbstractDescribableImpl<AdditionalCredentials> {


No, you must leave this class where it was for reasons of settings compatibility. Nothing is stopping SubversionSCMSource from reusing it here.

jglick · 2017-06-21T18:39:14Z

src/main/java/jenkins/scm/impl/subversion/SubversionSCMSource.java

    }

    @DataBoundConstructor
    @SuppressWarnings("unused") // by stapler
-    public SubversionSCMSource(String id, String remoteBase) {
+    public SubversionSCMSource(String id, String remoteBase, List<AdditionalCredentials> additionalCredentials) {


No, this should be in a @DataBoundSetter only.

jglick · 2017-06-21T18:40:43Z

src/main/java/jenkins/scm/impl/subversion/SubversionSCMSource.java

+        public boolean isDeterministic() {
+            //non-deterministic when using externals, to force Jenkins to recurse into subdirectories
+            if(usesExternals) {
+                return false;


Nondeterministic revisions are acceptable only for SCMs which inherently cannot support determinism, like CVS I suppose (though even there -D timestamps may offer a solution). In the case of Subversion with externals, we would rather need to extend SCMRevisionImpl to encode the revision number in each external encountered.

jglick · 2017-06-21T18:41:31Z

src/main/java/jenkins/scm/impl/subversion/SubversionSCMSource.java

@@ -273,7 +294,7 @@ protected SCMRevision retrieve(@NonNull SCMHead head, @NonNull TaskListener list
            String repoPath = SubversionSCM.DescriptorImpl.getRelativePath(repoURL, repository.getRepository());
            String path = SVNPathUtil.append(repoPath, head.getName());
            SVNRepositoryView.NodeEntry svnEntry = repository.getNode(path, -1);
-            return new SCMRevisionImpl(head, svnEntry.getRevision());
+            return new SCMRevisionImpl(head, svnEntry.getRevision(), (additionalCredentials.size()>0));


This is not precise. Additional credentials might have been specified but not actually used. More plausibly, the server may use externals which simply do not need any distinct credentials.

r-funke · 2017-06-22T11:01:16Z

@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?

oleg-nenashev

This PR introduces a breaking change + additionalCredentials handling seems to be error-prone and redundant in some cases.

I would also expect to see a test suite for the change

oleg-nenashev · 2017-06-23T06:16:23Z

src/main/java/hudson/scm/SubversionSCM.java

@@ -297,7 +297,7 @@ public SubversionSCM(List<ModuleLocation> locations,
     */
    public SubversionSCM(List<ModuleLocation> locations,
            boolean useUpdate, SubversionRepositoryBrowser browser, String excludedRegions, String excludedUsers, String excludedRevprop, String excludedCommitMessages) {
-    	this(locations, useUpdate, false, browser, excludedRegions, excludedUsers, excludedRevprop, excludedCommitMessages);
+        this(locations, useUpdate, false, browser, excludedRegions, excludedUsers, excludedRevprop, excludedCommitMessages);


May cause merge conflicts without a reason

oleg-nenashev · 2017-06-23T06:19:50Z

src/main/java/jenkins/scm/impl/subversion/SubversionSCMSource.java

@@ -121,6 +122,8 @@
    private final String remoteBase;

    private String credentialsId = ""; // TODO null would be a better default, but need to check null safety on usages
+
+    private List<AdditionalCredentials> additionalCredentials;


from what I see it still may be null when you read data from the disk

oleg-nenashev · 2017-06-23T06:21:03Z

src/main/java/jenkins/scm/impl/subversion/SubversionSCMSource.java

+    }
+    //without getter jelly will crash
+    public List<AdditionalCredentials> getAdditionalCredentials() {
+        return(additionalCredentials);


FindBugs will grumble about exposing external implementation. Either make it @Restricted for Jelly only or wrap with unmodifyableList

Thank you for your comments. How do I make it restricted? Does adding "@restricted(NoExternalUse.class)" work?

oleg-nenashev · 2017-06-23T06:21:35Z

src/main/java/jenkins/scm/impl/subversion/SubversionSCMSource.java


-        public SCMRevisionImpl(SCMHead head, long revision) {
+        public SCMRevisionImpl(SCMHead head, long revision, boolean usesExternals) {


Breaking change, the class was public

oleg-nenashev · 2017-06-23T06:23:36Z

src/main/java/hudson/scm/SubversionSCM.java

+    /**
+     * Convenience constructor, especially during testing.
+     */
+    public SubversionSCM(String[] svnUrls, String[] credentialIds, String[] locals, List<AdditionalCredentials> additionalCredentials) {


I do not believe new methods are really required if you implement proper null handling in the original constructors. You say these methods are useful for testing, but there is no autotests

oleg-nenashev · 2017-06-23T06:24:45Z

src/main/java/jenkins/scm/impl/subversion/SubversionSCMSource.java

@@ -136,15 +139,18 @@ public SubversionSCMSource(String id, String remoteBase, String credentialsId, S
        setCredentialsId(credentialsId);
        setIncludes(StringUtils.defaultIfEmpty(includes, DescriptorImpl.DEFAULT_INCLUDES));
        setExcludes(StringUtils.defaultIfEmpty(excludes, DescriptorImpl.DEFAULT_EXCLUDES));
+        this.additionalCredentials =  new ArrayList<AdditionalCredentials>();


Usage of this collection is ineffective if you do not plan to modify it on the flight. Use Collections.emptyList() instead in the code

…ersionSCMSource

r-funke · 2017-06-27T13:23:16Z

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.

oleg-nenashev · 2017-07-04T10:19:40Z

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 · 2017-07-04T10:20:52Z

Separate pull requests would be better though

regarding non-deterministic revision

r-funke · 2017-07-07T13:12:29Z

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

oleg-nenashev · 2017-07-07T14:35:30Z

src/main/java/jenkins/scm/impl/subversion/SubversionSCMSource.java

+        if(additionalCredentials==null) {
+            return Collections.emptyList();
+        }
+        return(additionalCredentials);


would be better to just return Collections.unmodifyableList(additionalCredentials)

oleg-nenashev · 2017-07-07T14:36:25Z

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

r-funke · 2017-07-24T10:42:03Z

Any news on this issue?

DavidA2014 · 2017-09-04T10:01:24Z

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

jglick · 2017-09-13T16:16:43Z

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.

imakowski · 2018-03-04T20:36:24Z

When this will be released?

probably obsolete

jglick previously requested changes Jun 21, 2017

View reviewed changes

moved AdditionalCredentials back to SubversionSCM

11db817

oleg-nenashev requested changes Jun 23, 2017

View reviewed changes

oleg-nenashev added needs-fix needs-jira-issue needs-testcase labels Jun 23, 2017

undo changes in SubversionSCM and more efficient initializing in Subv…

fa29350

…ersionSCMSource

r-funke changed the title ~~added support for AdditionalCredentials to SubversionSCMSource and fixed issues with externals~~ [JENKINS-35227] added support for AdditionalCredentials to SubversionSCMSource and fixed issues with externals Jun 27, 2017

made getter for additional credentials restricted and undid changes

3540bb6

regarding non-deterministic revision

oleg-nenashev removed needs-fix needs-jira-issue labels Jul 7, 2017

oleg-nenashev changed the title ~~[JENKINS-35227] added support for AdditionalCredentials to SubversionSCMSource and fixed issues with externals~~ [JENKINS-35227] added support for AdditionalCredentials to SubversionSCMSource Jul 7, 2017

oleg-nenashev reviewed Jul 7, 2017

View reviewed changes

returns unmodifiableList in getter for additionalCredentials

4a9730b

r-funke mentioned this pull request Jul 11, 2017

[JENKINS-45362] Workaround for not detecting changes in externals #191

Closed

jglick self-requested a review September 13, 2017 16:16

jglick removed their request for review March 5, 2018 22:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-35227] added support for AdditionalCredentials to SubversionSCMSource #189

[JENKINS-35227] added support for AdditionalCredentials to SubversionSCMSource #189

r-funke commented Jun 20, 2017

jglick Jun 21, 2017

jglick Jun 21, 2017

jglick Jun 21, 2017

jglick Jun 21, 2017

r-funke commented Jun 22, 2017

oleg-nenashev left a comment

oleg-nenashev Jun 23, 2017

oleg-nenashev Jun 23, 2017

oleg-nenashev Jun 23, 2017

r-funke Jun 23, 2017

oleg-nenashev Jul 4, 2017

oleg-nenashev Jun 23, 2017

oleg-nenashev Jun 23, 2017

oleg-nenashev Jun 23, 2017

r-funke commented Jun 27, 2017 •

edited

oleg-nenashev commented Jul 4, 2017

oleg-nenashev commented Jul 4, 2017

r-funke commented Jul 7, 2017

oleg-nenashev Jul 7, 2017

oleg-nenashev commented Jul 7, 2017

r-funke commented Jul 24, 2017

DavidA2014 commented Sep 4, 2017

jglick commented Sep 13, 2017

imakowski commented Mar 4, 2018


		public SCMRevisionImpl(SCMHead head, long revision) {
		public SCMRevisionImpl(SCMHead head, long revision, boolean usesExternals) {

[JENKINS-35227] added support for AdditionalCredentials to SubversionSCMSource #189

Are you sure you want to change the base?

[JENKINS-35227] added support for AdditionalCredentials to SubversionSCMSource #189

Conversation

r-funke commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-funke commented Jun 22, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-funke commented Jun 27, 2017 • edited

oleg-nenashev commented Jul 4, 2017

oleg-nenashev commented Jul 4, 2017

r-funke commented Jul 7, 2017

Choose a reason for hiding this comment

oleg-nenashev commented Jul 7, 2017

r-funke commented Jul 24, 2017

DavidA2014 commented Sep 4, 2017

jglick commented Sep 13, 2017

imakowski commented Mar 4, 2018

r-funke commented Jun 27, 2017 •

edited