Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: [JENKINS-48822] Better browser URL guessing #844

Closed
wants to merge 2 commits into from

Conversation

Gyanesha
Copy link

@Gyanesha Gyanesha commented Feb 24, 2020

JENKINS-48822 - Improve browser URL guessing

I have made the abstract BrowserInference Class which is the extension point and can be used by other GitRepository Browsers.

This is a work in progress. Do not merge

@MarkEWaite MarkEWaite changed the title Make git plugin browser URL guessing clearer and easier to understand [JENKINS-48822] Make git plugin browser URL guessing clearer and easier to understand Feb 24, 2020
@MarkEWaite MarkEWaite added the enhancement Improvement or new feature label Feb 24, 2020
@MarkEWaite MarkEWaite changed the title [JENKINS-48822] Make git plugin browser URL guessing clearer and easier to understand [JENKINS-48822] Better browser URL guessing Feb 24, 2020
@MarkEWaite
Copy link
Contributor

MarkEWaite commented Feb 24, 2020

I think the steps might be:

  • Add an extension (done in this pull request, not sure on the naming choice)
  • Write tests for that extension to confirm it behaves as initially implemented
  • Implement a method in the extension which takes the repository URL as an argument and returns either a GitRepositoryBrowser on a match or null
  • Write tests of the new method to confirm matching and non-matching cases
  • Extend the GitSCM constructor to iterate over all implementations of that extension, calling each extension with the repository URL, accepting the first GitRepositoryBrowser returned as the default
  • Implement the extension in one of the well-known existing repository browsers (GitHub, Bitbucket, GitLab, or Gitea)
  • Write tests of the extension implementation in that repository browser
  • Confirm the extension implementation in that repository browser works

@MarkEWaite MarkEWaite changed the title [JENKINS-48822] Better browser URL guessing WIP: [JENKINS-48822] Better browser URL guessing Feb 24, 2020
Copy link
Contributor

@fcojfernandez fcojfernandez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Gyanesha ! thanks for the contribution

I see we're far away from our goal, so let's be patient and eventually we'll get there working together. During all the process I will be requesting changes all the time so we don't merge the PR accidentally, ok?

First of all, please never open a PR from your forked master branch. That's a bad practice. You should create a branch based on master and rename it as JENKINS-488222 (just a suggestion for the name, but something self-explanatory)

Next, although Mark suggested to write the automated tests as the last point, in fact the best practice is to write them as the write of your code. It will help you to test your improvements and later you will not forget to write them.

So let's start:

  1. You've started from the 7th step Mark suggested you:

Define a Jenkins extension point for the browser inference logic inside the git plugin (see Defining an extension point)

I see the concept is not clear enough for you, so I will challenge you a bit with some questions/concerns. Hopefully, that will help you to get a better understanding

  1. Please start adding the tests so you can start testing you code


import hudson.plugins.git.browser.GitRepositoryBrowser;
import hudson.scm.RepositoryBrowsers;
import org.apache.tools.ant.ExtensionPoint;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As https://wiki.jenkins.io/display/JENKINS/Defining+a+new+extension+point mentions, you're using the wrong extension point class

Suggested change
import org.apache.tools.ant.ExtensionPoint;
import hudson.ExtensionPoint;

import hudson.scm.RepositoryBrowsers;
import org.apache.tools.ant.ExtensionPoint;

public abstract class BrowserInference extends ExtensionPoint {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you should wonder:

  1. Why abstract?
  2. Will you allow the rest of plugins to consume this extension? Or is it something that will be part only of the git plugin?
  3. Is some sort of UI needed? Will you offer a new UI?

If this extension is something that will never extended by other plugins, then:

  1. Annotate it as @Restricted(NoExternalUse.class) as in
    @Restricted(NoExternalUse.class) // stapler
  2. There is no need to make it abstract. Use abstract classes when you want them to be extended.

If there is some expected offered UI, you should implement the Descriptor. See the Describable/Descriptor pattern section in https://wiki.jenkins.io/display/JENKINS/Defining+a+new+extension+point

Comment on lines +15 to +21
public BrowserInference(GitRepositoryBrowser browser){
this.browser = browser;
}

String inferRepositoryBrowser(){
return "no-browser";
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be misunderstanding Mark's suggestion, but from his words my understanding is exactly the opossite:

I assume that the extension point would need a method that would be passed the repository URL and would either return an instance of GitRepositoryBrowser or null. The null return would mean that the extension rejected the request to provide a browser, while if the GitRepositoryBrowser was returned, then that would be the inferred browser.

You have to receive a String/URL object and return the proper GitRepositoryBrowser. The place for the logic to calculate/detect the URL is there. Returning the GitRepositoryBrowser you will have the same effect as when you select a browser. At this point, have you run a Jenkins with the plugin installed and have you seen the current behaviour? So I can sure we are on the same page and we're understanding each other.

@MarkEWaite MarkEWaite marked this pull request as draft May 14, 2020 21:54
@MarkEWaite
Copy link
Contributor

Closing this until someone decides to make further progress on it.

@MarkEWaite MarkEWaite closed this Dec 19, 2020
@Hrushi20
Copy link

Hey! I was looking into this issue.

  • Add an extension (done in this pull request, not sure on the naming choice)
  • Write tests for that extension to confirm it behaves as initially implemented
  • Implement a method in the extension which takes the repository URL as an argument and returns either a GitRepositoryBrowser on a match or null
  • Write tests of the new method to confirm matching and non-matching cases
  • Extend the GitSCM constructor to iterate over all implementations of that extension, calling each extension with the repository URL, accepting the first GitRepositoryBrowser returned as the default
  • Implement the extension in one of the well-known existing repository browsers (GitHub, Bitbucket, GitLab, or Gitea)
  • Write tests of the extension implementation in that repository browser
  • Confirm the extension implementation in that repository browser works

In the above points, I didn't understand the 5th point. Can someone explain me the 5th point.

Also is the aim of the issue to create a automatic guessable browser url?

@Hrushi20
Copy link

Hrushi20 commented Mar 30, 2022

Also can't I add the logic implemented for (Auto) option present in UI to the current Extension so that we can get the required GitBrowserRepository 🤔 ? Then this extension can be used in GitSCM to get the required browser url. Any suggestions or ideas?

@MarkEWaite
Copy link
Contributor

Hey! I was looking into this issue.

  1. Add an extension (done in this pull request, not sure on the naming choice)
  2. Write tests for that extension to confirm it behaves as initially implemented
  3. Implement a method in the extension which takes the repository URL as an argument and returns either a GitRepositoryBrowser on a match or null
  4. Write tests of the new method to confirm matching and non-matching cases
  5. Extend the GitSCM constructor to iterate over all implementations of that extension, calling each extension with the repository URL, accepting the first GitRepositoryBrowser returned as the default
  6. Implement the extension in one of the well-known existing repository browsers (GitHub, Bitbucket, GitLab, or Gitea)
  7. Write tests of the extension implementation in that repository browser
  8. Confirm the extension implementation in that repository browser works

In the above points, I didn't understand the 5th point. Can someone explain me the 5th point.

A new extension point will allow other plugins (like the Gitea plugin or the Tuleap branch source plugin or the Gerrit plugin) to provide their own implementation without modifying the git plugin. There are a few examples in the GitSCM class where the code iterates over all available extensions (for (GitSCMExtension ext : getExtensions())) and uses a result from that iteration.

I think that was the intent of that description

@Hrushi20
Copy link

I have a small doubt regarding the implementation. While setting up a new freestyle project, if I try to add git urls from different browsers i.e one from github and another from gitlab, then what should I select as the GitRepositoryBrowser???

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement or new feature work-in-progress
Projects
None yet
4 participants