jenkinsci
Init BrowserGuessExtension
JENKINS-48822 - Make git plugin browser URL guessing clearer and easier to understand
Types of changes
- [x] New feature (non-breaking change which adds functionality)
Further comments
I created an Extension which takes in an input string url and returns GitRepositoryBrowser. Not sure how to register the extension in GitSCM class.
The current extension is not being displayed while iterating through the extension list present in GitSCM.
private DescribableList<GitSCMExtension,GitSCMExtensionDescriptor> extensions;
GitSCM should find the matching BrowserGuesser from the ExtensionList returned by BrowserGuesser.all(), not from its own DescribableList.
If the browser has been configured by adding a GitBrowserSCMSourceTrait to the SCMSource, then GitBrowserSCMSourceTrait.decorateBuilder calls GitSCMBuilder.withBrowser, and GitSCMBuilder.build() passes the browser to GitSCM. GitSCM.getBrowser() then returns it, and SCM.getEffectiveBrowser() does not call guessBrowser(). So, it does not matter what the BrowserGuesser code would do in this case, because it won't be called anyway.
The guess browser method is only for guessing the browser if not selected specifically in the git-plugin UI. Is the aim of the PR to remove the previous implementation and use this extension to initialized all the Git Browsers?
I think this PR is going in the right direction; the BrowserGuesser extension point should be used only if the repository browser was not already configured in a different way.
Quoting from JENKINS-48822:
- For regular SCM in old-style jobs... if you see the url as well-known, and the user has not configured a browser, then replace their no-browser with the well-known one in the constructor
- We don't need to do anything for SCMSource impls as they have their own pre-configured subclasses that do the right thing
- For the
gitpipeline step, have it do the inference and log the inference to the build log- Make the inference logic an ExtensionPoint
Point 1 needs work in the GitSCM constructor. Point 3 needs work in GitStep. GitStep may be able to let the GitSCM constructor choose the browser, but GitStep should then log the result to a TaskListener. You have almost finished point 4 about ExtensionPoint. Only the code that calls the extensions is missing.
Finished Point 1 and Point 3. I called the guess browser in the constructor when the user has not configured a browser. This is replace it with a suitable Browser Object.
In point 3, I imagine the git pipeline step could log something like this to the TaskListener:
Selected the default "githubweb" repository browser for "https://github.com/jenkinsci/git-plugin/".
English is not my first language though; there is probably a better way to word this. Anyway, it should help the user understand which repository browser was chosen, why it was chosen, and that it can be changed if the default is not good.
Do we need to log in GitStep class only? Won't it be easier to log the status in the GitSCM constructor as we have the required data. What do you think?
The constructors of GitSCM don't have a TaskListener parameter, so I don't think they can log to the console log of the build, where users would expect to see the log entry.
It would be possible to call LOGGER.log, but those log entries go to ${JENKINS_URL}/log/, which is more difficult for users to find. Also, I think reading the logs from there requires the Overall/SystemRead permission. ("This permission grants read-only access to large parts of the overall system configuration.")
I understand that GitSCM doesn't have TaskListener, however the GitStep class also doesn't contains a TaskListener. Another thing to keep in mind is where to write the log. StreamTaskListener takes file path as its input in the constructor. Is there any default file present to which we need to write the logs?
GitStep.createSCM() can be called from at least these places:
-
SCMStep.StepExecutionImpl.run(), which gets a TaskListener by calling
ctx.get(TaskListener.class). However, this uses the SCM only for getting environment variables, so the repository browser doesn't seem worth logging here. -
SCMStep.checkout, which has a TaskListener parameter. This is unfortunately a
public finalmethod so GitSCMStep cannot override it.
Perhaps the Pipeline: SCM Step Plugin could be changed to pass the TaskListener as a parameter to the SCMStep.checkout method. I'm not sure exactly how to do this while preserving compatibility with other plugins that define classes that extend SCMStep and override checkout() without parameters, and other plugins that call SCMStep.checkout() without arguments. The Util.isOverridden method might help somehow.
Alternatively. perhaps the repository guessing could be postponed to GitSCM.checkout, which has a TaskListener parameter, instead of doing it in the GitSCM constructor.
Or, perhaps GitSCMStep could override StepExecution start(StepContext context) to guess the repository browser and save it to a field of GitSCMStep before calling super.start. This would be able to call context.get(TaskListener.class). Then GitSCMStep.createSCM() could read the repository browser from the field.
I'm not really happy with any of these options.
Okay! I understood what are the possible solutions right now.
Alternatively. perhaps the repository guessing could be postponed to GitSCM.checkout, which has a TaskListener parameter, instead of doing it in the GitSCM constructor.
What exactly is checkout in this context? Is it related to creating/updating the configuration of a new Jenkins job? If yes, I feel it is okay to add the logs here. Else it wouldn't make sense to add logs here and it won't match the context. Also it is easy to implement :point_up:
Or, perhaps GitSCMStep could override StepExecution start(StepContext context) to guess the repository browser and save it to a field of GitSCMStep before calling super.start. This would be able to call context.get(TaskListener.class). Then GitSCMStep.createSCM() could read the repository browser from the field.
We can also proceed with this implementation as it won't create any breaking changes. Overriding and implementing the logs would be an easier. However, if we use this approach, we will then have to remove the BrowserGuesser code present in the GitSCM constructor and move it into GitStep class. The guessBrowser method also would be deleted from the GitSCM class and added to GitStep class. This means that the BrowserGuess would be free from the GitSCM class and would be called before initializing GitSCM.
Not sure which method to proceed with :thinking: :thinking: :thinking:
GitSCM.checkout runs "git fetch" to fetch commits from the Git server to a local repository in a workspace, and then "git checkout" to get the files of a commit to the workspace so that they can be built. Or it does the equivalent things using JGit rather than the git executable. It also does other stuff. Anyway, it has a Run<?, ?> build parameter and does not tolerate that being null, so it can only be called during a build, not yet when a user is configuring a project.
So -- if the repository browser guessing is postponed to GitSCM.checkout, then I think the guessed repository browser won't show up in the configuration UI where we want it.
Maybe instead the GitSCM constructor could save the repository-guessing log as a string in a field of GitSCM, and then GitSCM.checkout could dump that string to the TaskListener. The string would have to be erased if something calls setBrowser. This approach seems somewhat hacky.
Another option is:
- Add GitStep.createSCM(TaskListener) and make that log the guessed repository browser.
- Make GitStep.createSCM() call this.createSCM(TaskListener.NULL).
- Copy SCMStep.StepExecutionImpl to GitStep.StepExecutionImpl and make it pass the TaskListener to createSCM.
- Make GitStep override start(StepContext context) so that it uses GitStep.StepExecutionImpl rather than SCMStep.StepExecutionImpl.
This would require some duplication of code, but not very much, and all the changes would be in GitStep and would not be able to cause surprises elsewhere. There would be no new fields and thus no risk of leaving out-of-date values in such fields. The main risk would be that any future changes in SCMStep.StepExecutionImpl might then need similar changes in GitStep.StepExecutionImpl.
However, if we use this approach, we will then have to remove the BrowserGuesser code present in the GitSCM constructor and move it into GitStep class.
Why? If GitStep guesses the browser and logs that it did so, then it passes a non-null RepositoryBrowser to the GitSCM constructor, and GitSCM won't try to guess the browser again even if it supports guessing.
If the URL doesn't match any known hosting site, then there will be a bit of wasted work as both GitStep and GitSCM will try to guess a browser. I doubt that would cost a lot of time, but if it is a problem, then it can be fixed by adding a package-private constructor to GitSCM. GitStep is a final class so other plugins won't extend GitStep and override the createSCM method to create an instance of anything that extends GitSCM.
When exactly is the start method called in the GitStep when override? I tried printing smtng in the start method but it doesn't gets printed in the terminal. I am also not able to print anything from the GitStep.createSCM(). I am not sure why this is happening. In the UI I created a freestyle project and configured all the data, but I can't see any of the printed lines.
GitStep.start should be called when the git step is executed in a pipeline. I'm not sure about freestyle projects.
How do you print from the start method; do you use System.out.print, context.get(TaskListener.class).getLogger().print, or something else?
It seems impossible to add GitStep as a build step in a freestyle project. Script Console shows that there is no jenkins.plugins.git.GitStep.DescriptorImpl instance in the list that hudson.tasks.Builder.all() returns. That causes config-builders.jelly not to display it in the "Add build step" dropdown list.
org.jenkinsci.plugins.workflow.steps.StepDescriptor.all() however returns a list that includes a jenkins.plugins.git.GitStep.DescriptorImpl instance.
How do you print from the start method; do you use System.out.print, context.get(TaskListener.class).getLogger().print, or something else?
I am using System.out.println to log. Can you tell me the steps to follow in the UI to trigger the log statements. I created a Jenkins pipeline project and saved it, still the logs are not being displayed
- First I opened Jenkins on Port 8080
- Then I created a new pipeline job using SCM by clicking on Pipeline script from SCM. Here I entered the github url and saved the configuration.
After the above steps I expected the Logs to be printed using System.out.println.
Else I can implement the below steps to finish logging the status in GitSCM class.
GitSCM.checkout runs "git fetch" to fetch commits from the Git server to a local repository in a workspace, and then "git checkout" to get the files of a commit to the workspace so that they can be built. Or it does the equivalent things using JGit rather than the git executable. It also does other stuff. Anyway, it has a Run, ?> build parameter and does not tolerate that being null, so it can only be called during a build, not yet when a user is configuring a project.
So -- if the repository browser guessing is postponed to GitSCM.checkout, then I think the guessed repository browser won't show up in the configuration UI where we want it.
Maybe instead the GitSCM constructor could save the repository-guessing log as a string in a field of GitSCM, and then GitSCM.checkout could dump that string to the TaskListener. The string would have to be erased if something calls setBrowser. This approach seems somewhat hacky.
To test changes of GitStep, I believe you need to have a git step in the pipeline and then run that pipeline. Something like this:
pipeline {
agent any
stages {
stage('Checkout') {
steps {
git 'https://github.com/jenkinsci/git-plugin/'
}
}
}
}
For this, it does not matter whether the pipeline itself is in Git.
[ERROR] hudson.plugins.git.GitSCMBrowserTest.guessedBrowser[[email protected]:/jenkinsci/git-plugin] Time elapsed: 0 s <<< ERROR! java.lang.IllegalStateException: Jenkins.instance is missing. Read the documentation of Jenkins.getInstanceOrNull to see what you are doing wrong. at jenkins.model.Jenkins.get(Jenkins.java:773) at hudson.plugins.git.browser.BrowserGuesser.all(BrowserGuesser.java:12) at hudson.plugins.git.browser.BrowserGuesser.getBrowser(BrowserGuesser.java:16) at hudson.plugins.git.GitSCM.guessBrowser(GitSCM.java:402) at hudson.plugins.git.GitSCM.
(GitSCM.java:210) at hudson.plugins.git.GitSCM. (GitSCM.java:176) at hudson.plugins.git.GitSCMBrowserTest.guessedBrowser(GitSCMBrowserTest.java:130) at jdk.internal.reflect.GeneratedMethodAccessor14.invoke(Unknown Source) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
I want to add tests to the extension. Any tips on what all I need to test? I already added the tests for checking the GitRepositoryBrowser Type. However the above error is being displayed while running mvn test
I'm not very familiar with writing tests for Jenkins plugins. The Plugin Utilities API plugin has JenkinsFacade (javadoc) for testing but maybe it's not a good idea to make the Git plugin depend on that. If you can run the test via https://github.com/jenkinsci/acceptance-test-harness then I suppose you can make Jenkins.get() work.
Or perhaps JenkinsRule is what you need here. It's used in GitSCMExtensionTraitTest.java, which calls SCMSourceTrait.all(), which calls SCMTrait.all, which likewise calls Jenkins.get(). So this appears to be a way to make Jenkins.get() work in a test.
This is the code for testing BrowserGuesser. On line 25, the extensions of type BrowserGuesser are being printed. The BrowserGuesser.all() similar to SCMTrait.all(), calls Jenkins.get().getExtensionList(BrowserGuesser.class);
The error stated above still persists. Also I see new errors in the Github ci/cd tests.
Thanks. I realized the error. The error was in GitSCMBrowserTest. I fixed it by adding
@ClassRule
public static JenkinsRule j = new JenkinsRule();
Can I remove the testGuessBrowser test present in GitHubWebTest? Else I need to add the below code to the same test.
@ClassRule
public static JenkinsRule j = new JenkinsRule();
I wonder if this change will cause trouble to other plugins that create instances of GitSCM during their own tests. They would then likewise have to create a JenkinsRule.
Yes! This will create a breaking change. How do we proceed with this? Are there any alternate solutions to avoid a breaking change?