Adds a flag to the clean after checkout and clean before checkout adv…#449
Adds a flag to the clean after checkout and clean before checkout adv…#449roguishmountain wants to merge 4 commits intojenkinsci:masterfrom
Conversation
|
Depends on jenkinsci/git-client-plugin#222 |
leandro-lucarella-sociomantic
left a comment
There was a problem hiding this comment.
Same comments as with jenkinsci/git-client-plugin#222
|
I uploaded a build of required git client plugin build and the git plugin build. The git client is based on the latest master branch. The git plugin is the latest master branch plus a browser guesser enhancement and a fix for a performance problem due to excessive calls to rev-parse. |
MarkEWaite
left a comment
There was a problem hiding this comment.
Changes look good in my interactive testing. Looking for further testing from @leandro-lucarella-sociomantic
|
This pull request works well for Freestyle jobs. It needs more work on the Pipeline portion of the implementation before it can be merged. The Pipeline syntax suggestions are not as tidy as they should be when submodule cleaning is enabled. |
|
For what is worth (and sorry for the delay), the tests also were well on our side, but we didn't test with pipelines. |
|
What's the current status of this PR? |
|
@MarkEWaite is the Pipeline work the only remaining issue before this PR can be merged? |
|
@akoeplinger the pipeline improvements are needed and further testing is needed. If you'd like to test the pull request, you can use https://ci.jenkins.io/job/Plugins/job/git-plugin/job/PR-449/107/artifact/org/jenkins-ci/plugins/git/4.0.0-rc2925.b198443241fc/git-4.0.0-rc2925.b198443241fc.hpi as the git plugin for testing along with https://ci.jenkins.io/job/Plugins/job/git-client-plugin/job/master/258/artifact/org/jenkins-ci/plugins/git-client/3.0.0-beta6-rc1827.ac5f9c819272/git-client-3.0.0-beta6-rc1827.ac5f9c819272.hpi as the git client plugin |
…anced options See bug report https://issues.jenkins-ci.org/browse/JENKINS-26660 for more details
007c189 to
f894f8d
Compare
|
@MarkEWaite the most recent commit fixes the pipeline syntax suggestions. |
|
|
||
| f.entry(title:_("Clean submodules (Add additional -f flag)"), field:"cleanSubmodule") { | ||
| f.checkbox() | ||
| } No newline at end of file |
There was a problem hiding this comment.
A newline should be added here.
|
|
||
| def f = namespace(lib.FormTagLib); | ||
|
|
||
| f.entry(title:_("Clean submodules (Add additional -f flag)"), field:"cleanSubmodule") { |
There was a problem hiding this comment.
There is a "cligit" and "jgit" implementation. This comment about the -f flag is more or less only valid for the "cligit" implementation. But I am not sure what's the common way to deal with the implementation differences regarding user facing documentation in this plugin. @MarkEWaite what's your opinion?
|
|
||
| private boolean cleanSubmodule; | ||
| @DataBoundConstructor | ||
| public CleanBeforeCheckout(final boolean cleanSubmodule) { |
There was a problem hiding this comment.
Please have a look at https://jenkins.io/doc/developer/plugin-development/pipeline-integration/#constructor-vs-setters. I suggest to not add another constructor (this procedure is hard to maintain when more and more parameters are added) but to use a setter method annotated with @DataBoundSetter.
There was a problem hiding this comment.
@roguishmountain did you have some time to look at this?
| @@ -0,0 +1,8 @@ | |||
| package hudson.plugins.git.extensions.impl.CleanBeforeCheckout; | |||
There was a problem hiding this comment.
Semicolons are optional in Groovy and it is common to not use them.
|
Was this resolved or abandoned? |
|
Neither resolved nor abandoned. I've assigned it to the milestone "Later" and closed the pull request for now. My priorities for development on the git plugin are:
Unless something else changes, I won't do further review on this change for a long time. |
|
|
||
| private boolean cleanSubmodule; | ||
| @DataBoundConstructor | ||
| public CleanCheckout(final boolean cleanSubmodule) { |
|
@roguishmountain can you have a look at the review comments and reopen this pull request? |
|
See #792 |
|
Merged into master as #792 . Planned for release in git plugin 4.1.0 |
…anced options
See bug report https://issues.jenkins-ci.org/browse/JENKINS-26660 for more details