[JENKINS-63519] Create a better mechanism to detect node-specific git implementations #949

rishabhBudhouliya · 2020-09-04T05:37:08Z

JENKINS-63519

Change the current way to suggest a git implementation, the modifications can be broadly described as:

Earlier, if the system wants to recommend jgit and jgit is not available in the system, it used to go for cli git. We've realised that that is unnecessary. Now the GitToolChooser would recommend nothing ("NONE") if it wants to recommend jgit or jgitapache but it is not enabled by the user.
For a repository size > 5 MiB: if the user enables a node-specific git tool (let's say git-for-windows), earlier the GitToolChooser used to recommend git whatever that may be perceived by the system, which is wrong.
Now, it will return the same node-specific git tool, git-for-windows.
For a repository size > 5 MiB: if the user enables jgit as a git tool, now the GitToolChooser has the responsibility of selecting the right implementation (maybe node-specific). With the help of recommendGitToolOnAgent it is able to do so.

Note: The current changes have made the decision process a little complex, I'm not proud of the current design and would want to shift to a builder pattern for the initialization of the class considering the increasing number of args required to initialise GitToolChooser.

Checklist

Types of changes

Bug fix (non-breaking change which fixes an issue)

In reference to the bug JENKINS-63519 which exposed an issue where the GitToolChooser is not able to determine the correct implementation if a node-specific installation is present in the system. This API is called when the system wants to recommend `cli git` and the user has chosen `jgit`. To find the right cli git, it iterates over the available installations, checks if one of them contains a property map, confirms with the node attributes (using GitUtils.resolveGitTool) and then finally suggests the right git implementation. With this change, we have added two new parameters: Node path and TaskListener. Both of them are needed for better estimation of the correct git tool present on a particular agent.

rishabhBudhouliya · 2020-09-04T05:45:16Z

src/main/java/hudson/plugins/git/GitSCM.java

@@ -838,6 +838,8 @@ public GitClient createClient(TaskListener listener, EnvVars environment, Run<?,

        String gitExe = getGitExe(n, listener);

+        GitTool gitTool = getGitTool(n, null, listener);


This was added to access the GitTool chosen by the system instead of the git executable path for GitToolChooser.

rishabhBudhouliya · 2020-09-04T05:49:22Z

src/main/java/jenkins/plugins/git/GitToolChooser.java

-        }
-        return recommendation;
-    }
-
    private GitTool resolveGitToolForRecommendation(GitTool userChoice, String recommendation) {
        GitTool tool;
        if (recommendation.equals(JGitTool.MAGIC_EXENAME)) {


For repository size < 5MiB, the recommendation should be jgit/jgitapache. This branch of logic will try to recommend jgit or jgitapache and confirm with the system if it is enabled by the user. It would return a null if it is not enabled by the user.

rishabhBudhouliya · 2020-09-04T05:51:16Z

src/main/java/jenkins/plugins/git/GitToolChooser.java

@@ -209,13 +172,48 @@ private GitTool resolveGitToolForRecommendation(GitTool userChoice, String recom
                return null;
            }
        } else {


This branch of logic is designed for repo size > 5M.
Here two things can happen:

if user's choice is not jgit or jgitapache, it would be a command line git tool. We would return that same choice without any concern.

if the user's choice is jgit or jgitapache, it would look for the right command line git tool using recommendGitToolOnAgent.

…hoice is JGit and the size of repo> 5M For the GitToolChooser, the most difficult scenario is when the user has chosen jgit and size of repo > 5M. In this case, the system has to decide which command line git implementation will be the right implementation for that particular environment without breaking any possible use-case. With the added test cases what we want to say is (keeping the above mentioned conditions same): - if there is a node specific installation present, that will be suggested after being verified that it is actually installed in that environment. - if there is no node specific installation, the default implementation will be suggested. There is one side-effect of this implementation, if the only implementation is `jgit` in the system then that will be recommended instead of command line git. But that wouldn't affect the user since there would be no possible change in the git implementation used for the git client.

MarkEWaite

Still need further review, but the javadoc update is one that can be done while other reviews are happening

MarkEWaite · 2020-09-05T14:21:40Z

src/main/java/jenkins/plugins/git/GitToolChooser.java

@@ -48,12 +52,14 @@
     * @throws IOException on error
     * @throws InterruptedException on error
     */
-    public GitToolChooser(String remoteName, Item projectContext, String credentialsId, String gitExe, Boolean useJGit) throws IOException, InterruptedException {
+    public GitToolChooser(String remoteName, Item projectContext, String credentialsId,


Please add javadoc comments for the new parameters. That will resolve the javadoc warnings below and keep the javadoc clean

MarkEWaite · 2020-09-05T14:28:24Z

src/main/java/jenkins/plugins/git/GitToolChooser.java

-                return determineToolName(gitExe, "git");
+                GitTool rTool = resolveGitToolForRecommendation(tool, "git");
+                if (rTool == null) {
+                    return "NONE";


This statement is never reached by the tests in GitToolChooserTest. Could it be reached reasonably by an addition of one of those tests?

Suggested change

return "NONE";

return "NONE";

Thanks for catching this. It can't be reached because there is no possible scenario where the tool will be null. I have removed it.

…ommended

MarkEWaite

Need to fix the Windows path references so that the tests pass on Windows and need to switch from SystemUtils to the isWindows() technique that is used elsewhere in the plugin.

See MarkEWaite@3763bb3 for a proposed change.

src/test/java/jenkins/plugins/git/GitToolChooserTest.java

Use isWindows() technique to maintain consistency across the plugin

rishabhBudhouliya · 2020-09-08T17:17:12Z

@MarkEWaite In reference to the test failures on Windows, I have found an interesting test which uses the same technique which I borrowed to create a ToolInstallation implementation.

Refer to:

git-plugin/src/test/java/jenkins/plugins/git/GitSCMSourceTest.java

Lines 363 to 366 in be265e0

    
           if (isWindows()) { // Runs on Unix only 
        
               /* Do not distract warnings system by using assumeThat to skip tests */ 
        
               return; 
        
           }

Those tests using a node specific Tool installation implementation seem to avoid Windows environment, stating those tests only run on Unix only.

I am assuming I do the same thing those tests are doing since this issue seems most likely a test configuration problem rather than the GitToolChooser's problem.

rishabhBudhouliya · 2020-09-08T17:29:57Z

@MarkEWaite In fact I just saw a recent commit from you: 6aff94f.

If it is okay, I'd like to apply the same technique over here to maintain the test warning count.

MarkEWaite · 2020-09-08T17:59:29Z

Those tests using a node specific Tool installation implementation seem to avoid Windows environment, stating those tests only run on Unix only.

I am assuming I do the same thing those tests are doing since this issue seems most likely a test configuration problem rather than the GitToolChooser's problem.

I like that idea. If others have deduced that a portion of the test framework is platform specific, it is very reasonable for us to limit ourselves to that platform.

Two of the test cases contains a node-specific tool installation implementation which can perform an installation only on Unix based platforms. Considering that shortcoming this change limits those tests to Unix based platforms only.

rishabhBudhouliya · 2020-09-08T20:09:25Z

Oops, tried a forced rebuild by closing and reopening the PR.

MarkEWaite · 2020-09-11T02:25:21Z

Fix for the failing test is included in #952 . Sorry for the distraction from your fix.

rishabhBudhouliya added 2 commits September 2, 2020 00:01

Resolve JENKINS-63519

f921410

rishabhBudhouliya commented Sep 4, 2020

View reviewed changes

rishabhBudhouliya added 4 commits September 4, 2020 12:10

Add better assertion for unit test

1ceed4d

Remove assertion of NONE to jgit for testGitToolWithOnlyJGit

193b7e5

Fix another test assertion statement

34093c3

MarkEWaite requested changes Sep 5, 2020

View reviewed changes

MarkEWaite reviewed Sep 5, 2020

View reviewed changes

rishabhBudhouliya added 2 commits September 5, 2020 20:04

Remove unreachable check condition in case of command line git is rec…

25239d1

…ommended

Add missing params for javadoc

43c3094

MarkEWaite requested changes Sep 5, 2020

View reviewed changes

src/test/java/jenkins/plugins/git/GitToolChooserTest.java Outdated Show resolved Hide resolved

src/test/java/jenkins/plugins/git/GitToolChooserTest.java Outdated Show resolved Hide resolved

rishabhBudhouliya added 3 commits September 7, 2020 11:32

Fix Windows specific tests for git tool, use isWindows()

7bc2b84

Use isWindows() technique to maintain consistency across the plugin

Fix Windows specific paths

cfa0661

Add javadoc for getResolvedGitTool

2f985e3

Merge branch 'master' into JENKINS-63519

c82a734

Ignore platform specific test cases on Windows

34a146d

Two of the test cases contains a node-specific tool installation implementation which can perform an installation only on Unix based platforms. Considering that shortcoming this change limits those tests to Unix based platforms only.

rishabhBudhouliya closed this Sep 8, 2020

rishabhBudhouliya reopened this Sep 8, 2020

MarkEWaite approved these changes Sep 11, 2020

View reviewed changes

MarkEWaite merged commit b3ed2a1 into jenkinsci:master Sep 12, 2020

MarkEWaite added the bugfix Fixes a bug - used by Release Drafter label Sep 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-63519] Create a better mechanism to detect node-specific git implementations #949

[JENKINS-63519] Create a better mechanism to detect node-specific git implementations #949

rishabhBudhouliya commented Sep 4, 2020 •

edited

Loading

rishabhBudhouliya Sep 4, 2020

rishabhBudhouliya Sep 4, 2020

rishabhBudhouliya Sep 4, 2020

MarkEWaite left a comment

MarkEWaite Sep 5, 2020

rishabhBudhouliya Sep 5, 2020

MarkEWaite Sep 5, 2020 •

edited

Loading

rishabhBudhouliya Sep 5, 2020

MarkEWaite left a comment

rishabhBudhouliya commented Sep 8, 2020

rishabhBudhouliya commented Sep 8, 2020

MarkEWaite commented Sep 8, 2020

rishabhBudhouliya commented Sep 8, 2020

MarkEWaite commented Sep 11, 2020

		@@ -838,6 +838,8 @@ public GitClient createClient(TaskListener listener, EnvVars environment, Run<?,

		String gitExe = getGitExe(n, listener);

		GitTool gitTool = getGitTool(n, null, listener);

[JENKINS-63519] Create a better mechanism to detect node-specific git implementations #949

[JENKINS-63519] Create a better mechanism to detect node-specific git implementations #949

Conversation

rishabhBudhouliya commented Sep 4, 2020 • edited Loading

JENKINS-63519

Checklist

Types of changes

rishabhBudhouliya Sep 4, 2020

Choose a reason for hiding this comment

rishabhBudhouliya Sep 4, 2020

Choose a reason for hiding this comment

rishabhBudhouliya Sep 4, 2020

Choose a reason for hiding this comment

MarkEWaite left a comment

Choose a reason for hiding this comment

MarkEWaite Sep 5, 2020

Choose a reason for hiding this comment

rishabhBudhouliya Sep 5, 2020

Choose a reason for hiding this comment

MarkEWaite Sep 5, 2020 • edited Loading

Choose a reason for hiding this comment

rishabhBudhouliya Sep 5, 2020

Choose a reason for hiding this comment

MarkEWaite left a comment

Choose a reason for hiding this comment

rishabhBudhouliya commented Sep 8, 2020

rishabhBudhouliya commented Sep 8, 2020

MarkEWaite commented Sep 8, 2020

rishabhBudhouliya commented Sep 8, 2020

MarkEWaite commented Sep 11, 2020

rishabhBudhouliya commented Sep 4, 2020 •

edited

Loading

MarkEWaite Sep 5, 2020 •

edited

Loading