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

Add ability to configure git tool, repository browser, and additional behaviours for literate/multi-branch projects. #350

Merged
merged 4 commits into from Jun 25, 2016

Conversation

@wadahiro
Copy link

wadahiro commented Sep 8, 2015

This can be useful for some jobs for multi-branch projects (such as literate plugin and multi-branch project plugin).

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Sep 8, 2015

@jglick and @ndeloof I need your help to evaluate this pull request. I'm not a credible reviewer of a proposal to extend the git plugin to assist with workflow.

@ndeloof

This comment has been minimized.

Copy link
Member

ndeloof commented Sep 8, 2015

@wadahiro can you prepare a PR on literate/multi-branch to demonstrate use of this ?
It's not clear to me how this is supposed to be used. Not opposed to a merge, but seems unnecessary from first overview

@wadahiro wadahiro changed the title Added options to GitSCMSource for setting GitTool, RepositoryBrowser, and Additional Behaviours. Add ability to configure git tool, repository browser, and additional behaviours for iterate/multi-branch projects. Sep 8, 2015
@wadahiro

This comment has been minimized.

Copy link
Author

wadahiro commented Sep 8, 2015

@ndeloof I changed this PR description. How about this?

@jenkinsadmin

This comment has been minimized.

Copy link
Member

jenkinsadmin commented Sep 8, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@ndeloof

This comment has been minimized.

Copy link
Member

ndeloof commented Sep 8, 2015

@wadahiro the description is fine, this is not what I was asking for. Changes looks good, but why they are needed to is not clear to me, so a sample usage from another plugin would help.

@wadahiro

This comment has been minimized.

Copy link
Author

wadahiro commented Sep 8, 2015

@ndeloof
I am sorry for not explaining sufficiently.

why they are needed to is not clear to me,

Some plugins configure git configurations via scm-api (the implementation is GitSCMSource). As far as I know, Literate plugin and Multi-branch project plugin use this scm-api. Because of GitSCMSource doesn't support more detailed settings such as 'Additional Behaviors' currently, we cannot use useful options such as 'Shallow clone'.

so a sample usage from another plugin would help.

In Multi-branch project plugin, it includes 'descriptor.configPage' of SCMSource simply in the config jelly. So if GitSCMSource supports more options, it can be generate more options automatically.

@ndeloof

This comment has been minimized.

Copy link
Member

ndeloof commented Sep 8, 2015

got it

LGTM

@wadahiro

This comment has been minimized.

Copy link
Author

wadahiro commented Oct 7, 2015

@ndeloof When will this PR be merged? Has there been any issue with this PR itself? Please let me know if there are any problems.

@ndeloof

This comment has been minimized.

Copy link
Member

ndeloof commented Oct 7, 2015

I'm not maintaining this plugin anymore

@wadahiro

This comment has been minimized.

Copy link
Author

wadahiro commented Oct 7, 2015

@MarkEWaite Could you evaluate this pull request? Please let me know if there are any problems.

@DataBoundConstructor
public GitSCMSource(String id, String remote, String credentialsId, String includes, String excludes, boolean ignoreOnPushNotifications) {
public GitSCMSource(String id, String remote, String credentialsId, String includes, String excludes,
boolean ignoreOnPushNotifications, GitRepositoryBrowser browser, String gitTool,

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Oct 7, 2015

Member

Use @DataBoundSetters ?

This comment has been minimized.

Copy link
@wadahiro

wadahiro Oct 8, 2015

Author

@KostyaSha Thanks. I'll fix to use @DataBoundSetter against three new args(browser, gitTool, extensions).

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Oct 8, 2015

I hope to evaluate this pull request (code review and interactive test) within the next week. Thanks for your patience.

@jason-womack

This comment has been minimized.

Copy link

jason-womack commented Dec 14, 2015

@MarkEWaite What is the status of this PR?

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Dec 15, 2015

It will likely be many weeks before this pull request is evaluated. My optimism in October has been tempered by a very heavy schedule at work.

Since there are now conflicts between this pull request and the master branch, the original author could resolve the conflicts with a merge from the master branch. That would help speed the process a little

@bgianfo

This comment has been minimized.

Copy link

bgianfo commented Feb 15, 2016

@wadahiro Any hope of syncing this up to master?

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Feb 15, 2016

Yes, there's hope. The merge conflict is trivial enough that I can resolve it myself. I doubt it will be on the master today, but it may be on the master within the next 1-2 weeks.

@bgianfo

This comment has been minimized.

Copy link

bgianfo commented Feb 27, 2016

Anything I can do to help?

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Feb 27, 2016

Sure @bgianfo , you can help by building a copy of this pull request and deploying it. Test it in various ways to confirm that it does not break existing use models, then post your results with the pull request. I'm much more comfortable when others can say that they've used a pull request successfully.

wadahiro added 4 commits Sep 2, 2015
and Additional Behaviours. This can be useful for some jobs for
multi-branch projects (such as literate plugin and multi-branch project
plugin).
@wadahiro wadahiro force-pushed the wadahiro:add-options-to-gitscmsource branch from f976f92 to fda4f12 Apr 19, 2016
@wadahiro

This comment has been minimized.

Copy link
Author

wadahiro commented Apr 19, 2016

@MarkEWaite I resolved the merge conflict.
I have been using this patched version with Multi-branch project plugin for six months in the several projects, but there was no problem.

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Apr 19, 2016

@wadahiro thanks for resolving the merge conflicts. I'm glad to hear that it has been working well for you. It will be several weeks (at least) before I can evaluate this.

I'm traveling on business this week and won't have much time for Jenkins work while I'm traveling.

I will start the automated evaluation in my environment later today so that I can at least report if the automated tests were successful in the multi-platform / multi-version automated test environment.

@MarkEWaite MarkEWaite changed the title Add ability to configure git tool, repository browser, and additional behaviours for iterate/multi-branch projects. Add ability to configure git tool, repository browser, and additional behaviours for literate/multi-branch projects. Apr 19, 2016
@wadahiro

This comment has been minimized.

Copy link
Author

wadahiro commented Jun 8, 2016

@MarkEWaite Any updates?

@MarkEWaite MarkEWaite merged commit 38a3b04 into jenkinsci:master Jun 25, 2016
1 check passed
1 check passed
Jenkins This pull request looks good
Details
@wadahiro

This comment has been minimized.

Copy link
Author

wadahiro commented Jun 25, 2016

@MarkEWaite Thanks!! 😄

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jul 4, 2016

This change breaks the binary compatibility and causes issues like JENKINS-36419. All abstract methods need to be reworked

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jul 4, 2016

Working on the fix, but such interface is going to be a headache. Seems I'll have to just move some methods upstairs and break the compatibility for setters

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jul 4, 2016

@wadahiro Do you use this functionality anywhere?

if (extensions == null) {
extensions = new ArrayList<GitSCMExtension>();
}
return extensions;

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 4, 2016

Member

This method exposes internal implementation. FindBugs should not have accepted it. CC @MarkEWaite (is it still disabled?)

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Jul 4, 2016

Yes, it is still disabled. I'll check it more rigorously in the future. Sorry about that!

@wadahiro

This comment has been minimized.

Copy link
Author

wadahiro commented Jul 5, 2016

@oleg-nenashev @MarkEWaite I apologize for the breaking change. 🙇 I shouldn't have add abstract methods into AbstractGitSCMSource class. Thank you for your great works!

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jul 12, 2016

Does this not purport to fix JENKINS-31924?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.