Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-71955

GitLab branch source scan fails with null pointer exception if credentials are not provided

    • Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: Major Major

      A GitLab branch source project fails its initial scan with a null pointer exception. The message is:

      Checking project markewaite-jenkins-plugins/tasks-pipeline
      Proposing markewaite-jenkins-plugins/tasks-pipeline
      ERROR: Failed to create or update a subproject markewaite-jenkins-plugins/tasks-pipeline
      java.lang.NullPointerException: Cannot invoke "java.lang.Boolean.booleanValue()" because the return value of "org.gitlab4j.api.models.Project.getMergeRequestsEnabled()" is null
      	at io.jenkins.plugins.gitlabbranchsource.GitLabSCMSource.retrieve(GitLabSCMSource.java:334)
      	at jenkins.scm.api.SCMSource._retrieve(SCMSource.java:372)
      	at jenkins.scm.api.SCMSource.fetch(SCMSource.java:326)
      	at jenkins.branch.MultiBranchProjectFactory$BySCMSourceCriteria.recognizes(MultiBranchProjectFactory.java:262)
      	at jenkins.branch.OrganizationFolder$SCMSourceObserverImpl$1.recognizes(OrganizationFolder.java:1361)
      	at jenkins.branch.OrganizationFolder$SCMSourceObserverImpl$1.complete(OrganizationFolder.java:1376)
      	at jenkins.scm.api.trait.SCMNavigatorRequest.process(SCMNavigatorRequest.java:252)
      	at jenkins.scm.api.trait.SCMNavigatorRequest.process(SCMNavigatorRequest.java:202)
      	at io.jenkins.plugins.gitlabbranchsource.GitLabSCMNavigator.visitSources(GitLabSCMNavigator.java:305)
      	at jenkins.branch.OrganizationFolder.computeChildren(OrganizationFolder.java:535)
      	at com.cloudbees.hudson.plugins.folder.computed.ComputedFolder.updateChildren(ComputedFolder.java:269)
      	at com.cloudbees.hudson.plugins.folder.computed.FolderComputation.run(FolderComputation.java:167)
      	at jenkins.branch.OrganizationFolder$OrganizationScan.run(OrganizationFolder.java:920)
      	at hudson.model.ResourceController.execute(ResourceController.java:101)
      	at hudson.model.Executor.run(Executor.java:442)
      

      The job definition is available as job-definition.xml

          [JENKINS-71955] GitLab branch source scan fails with null pointer exception if credentials are not provided

          Manan added a comment -

          Hi markewaite , thanks for the quick response. imo the validation check for the existence of gitlab credentials should live somewhere in the [gitlab-branch-source-plugin|https://github.com/jenkinsci/gitlab-branch-source-plugin/tree/7dde70ed15224ec60c76b9699fc73ad4b6afbd5f.] 

          I think having a validation check while building the GitLab API, more precisely here would make sense. Would love to get your thoughts.

          Manan added a comment - Hi markewaite , thanks for the quick response. imo the validation check for the existence of gitlab credentials should live somewhere in the [gitlab-branch-source-plugin| https://github.com/jenkinsci/gitlab-branch-source-plugin/tree/7dde70ed15224ec60c76b9699fc73ad4b6afbd5f .]  I think having a validation check while building the GitLab API, more precisely here would make sense. Would love to get your thoughts.

          Mark Waite added a comment - - edited

          manan_goel I would look at the GitHub Branch Source plugin as a first example of the preferred technique. The GitHub Branch Source plugin shows the message "Credentials are recommended" when credentials are not provided, but then allows the user to continue without credentials.

          The Gitea plugin is another example you could use for the technique.

          I think that the GitLab branch source should strongly suggest use of a credential, but should not require it. That's how the other branch sources behave in Jenkins.

          Mark Waite added a comment - - edited manan_goel I would look at the GitHub Branch Source plugin as a first example of the preferred technique. The GitHub Branch Source plugin shows the message "Credentials are recommended" when credentials are not provided, but then allows the user to continue without credentials. The Gitea plugin is another example you could use for the technique. I think that the GitLab branch source should strongly suggest use of a credential, but should not require it. That's how the other branch sources behave in Jenkins.

          Manan added a comment -

          That makes sense, will take a look

          Manan added a comment - That makes sense, will take a look

          Manan added a comment -

          Hey markewaite, based on the Gitea plugin, I've come up with an implementation to validate credentials. I can't quite figure out how the frontend and the backend interact with each other. I was wondering if you could direct me in the right direction.

           

          Manan added a comment - Hey markewaite , based on the Gitea plugin, I've come up with an implementation to validate credentials. I can't quite figure out how the frontend and the backend interact with each other. I was wondering if you could direct me in the right direction.  

          Mark Waite added a comment - - edited

          manan_goel thanks for the implementation. That's a great start and that's just the way that I would have done it. Look at an existing implementation, reuse it as much as possible. Well done.

          I ran your code with the command:

          mvn clean -Djenkins.version=2.426.3 hpi:run
          

          There is an important typo in your implementation. Change doCheckCredentialId to doCheckCredentialsId and you'll then see that when creating a new GitLab multibranch Pipeline, it will prompt that a credential is recommended. With that change, I was able to see the "Credentials are recommended" message when I created a GitLab multibranch Pipeline without credentials.

          The same change will be needed for GitLab organization folders. When I use your code to create a GitLab organization folder, no message is displayed. I assume that means a doCheckCredentialsId implementation is needed in the organization folder class as well.

          You'll also need to update the code to allow empty credentials and not report a null pointer exception, but that can come later.

          Mark Waite added a comment - - edited manan_goel thanks for the implementation. That's a great start and that's just the way that I would have done it. Look at an existing implementation, reuse it as much as possible. Well done. I ran your code with the command: mvn clean -Djenkins.version=2.426.3 hpi:run There is an important typo in your implementation. Change doCheckCredentialId to doCheckCredentialsId and you'll then see that when creating a new GitLab multibranch Pipeline, it will prompt that a credential is recommended. With that change, I was able to see the "Credentials are recommended" message when I created a GitLab multibranch Pipeline without credentials. The same change will be needed for GitLab organization folders. When I use your code to create a GitLab organization folder, no message is displayed. I assume that means a doCheckCredentialsId implementation is needed in the organization folder class as well. You'll also need to update the code to allow empty credentials and not report a null pointer exception, but that can come later.

          Manan added a comment -

          Thanks for the quick review! I have replicated the same for gitlab organization folders as well and it is working! Is this where the tests for this would go ?

          For not reporting the null pointer exception, would we want to keep it within the scope of this PR?

          Manan added a comment - Thanks for the quick review! I have replicated the same for gitlab organization folders as well and it is working! Is this where the tests for this would go ? For not reporting the null pointer exception, would we want to keep it within the scope of this PR?

          Mark Waite added a comment - - edited

          Is this where the tests for this would go ?

          I'm not very confident in asserting where a test should be placed for the doCheckCredentialsId method. I'm not especially confident that writing an automated test of that method is a good use of your time. Some examples of automated tests of those types of methods are available in:

          If you can create an automated test, then that is a good thing. If it is terribly painful to create the automated tests, then I think the maintainers of the plugin might be persuaded to accept the change based on a description of the interactive testing that you performed.

          For not reporting the null pointer exception, would we want to keep it within the scope of this PR?

          I'm less concerned about whether it is in the scope of that pull request than whether it is in the scope of this issue. I think it is in the scope of this issue because the issue is specifically reporting a null pointer exception that is reported to the user. If you'd rather use two pull requests to resolve this issue, I think that is a reasonable choice. The pull request you're currently developing will improve the user experience without resolving the null pointer exception. That improvement is a very good thing. If you use a second pull request to avoid the null pointer exception, that is also a very good thing.

          Mark Waite added a comment - - edited Is this where the tests for this would go ? I'm not very confident in asserting where a test should be placed for the doCheckCredentialsId method. I'm not especially confident that writing an automated test of that method is a good use of your time. Some examples of automated tests of those types of methods are available in: AssemblaWebDoCheckURLTest UserRemoteConfigRefSpecTest NodeLabelBuildParameterTest If you can create an automated test, then that is a good thing. If it is terribly painful to create the automated tests, then I think the maintainers of the plugin might be persuaded to accept the change based on a description of the interactive testing that you performed. For not reporting the null pointer exception, would we want to keep it within the scope of this PR? I'm less concerned about whether it is in the scope of that pull request than whether it is in the scope of this issue. I think it is in the scope of this issue because the issue is specifically reporting a null pointer exception that is reported to the user. If you'd rather use two pull requests to resolve this issue, I think that is a reasonable choice. The pull request you're currently developing will improve the user experience without resolving the null pointer exception. That improvement is a very good thing. If you use a second pull request to avoid the null pointer exception, that is also a very good thing.

          Manan added a comment -

          Hey markewaite , I have raised a PR here. I'm continuing to try to automate the test for this, thanks for sharing examples for the same.

          I am going to pick up the Null pointer exception in a separate issue

          Manan added a comment - Hey markewaite , I have raised a PR here. I'm continuing to try to automate the test for this, thanks for sharing examples for the same. I am going to pick up the Null pointer exception in a separate issue

          Mark Waite added a comment -

          Very good manan_goel. I'm about to leave town for two weeks. I won't be much help until I return. Hopefully one of the maintainers of the GitLab branch source plugin will review the pull request.

          Mark Waite added a comment - Very good manan_goel . I'm about to leave town for two weeks. I won't be much help until I return. Hopefully one of the maintainers of the GitLab branch source plugin will review the pull request.

          Manan added a comment -

          Thanks for all the help so far! I really appreciate it!

          Manan added a comment - Thanks for all the help so far! I really appreciate it!

            Unassigned Unassigned
            markewaite Mark Waite
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated: