Make all CLI commands compatible with Pipeline where possible #2874

jglick · 2017-05-05T22:52:40Z

Description

Subsumes #2866 & #2694.

Changelog entries

Proposed changelog entries:

Fixed Pipeline compatibility for a number of CLI commands (delete-builds, list-changes, console, set-build-description, and set-build-display-name), as well as some issues affecting error reporting in other commands when used with Pipeline.

Desired reviewers

@reviewbybees & @jenkinsci/code-reviewers

…g deprecated Remoting-based CLI calls with CLICommandInvoker.

…key on just one!

…hile we are here.

…y for unknown reasons.

…sabled-JENKINS-27299

…sabled.jelly.

…riggerItem.

…o CLI-JENKINS-41527

ghost · 2017-05-06T00:23:24Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

oleg-nenashev

The code mostly looks good, but I am aware of some edge cases. Hence 🐛 until we discuss them at least

oleg-nenashev · 2017-05-07T20:58:27Z

core/src/main/java/hudson/cli/DeleteBuildsCommand.java

@@ -38,7 +37,7 @@
 * @author Kohsuke Kawaguchi
 */
 @Extension
-public class DeleteBuildsCommand extends AbstractBuildRangeCommand {
+public class DeleteBuildsCommand extends JobRangeCommand {


🐜 This is binary incompatible. The only usage outside the core: https://github.com/jenkinsci/matrix-project-plugin/blob/8d94ba80991cf84c361c5739bc7d9a427f9171ad/src/test/java/hudson/matrix/MatrixProjectTest.java#L680 . It seems to be safe.

I would consider adding "Restricted" or maybe creating a new class instead.

CLI command classes are not considered APIs. Could add @Restricted to emphasize that, I suppose.

CLI command classes are not considered APIs

By whom? No explicit documentation about that :(
But yes, Restricted would help

oleg-nenashev · 2017-05-07T20:59:39Z

core/src/main/java/hudson/cli/ListChangesCommand.java

@@ -20,7 +21,7 @@
 * @author Kohsuke Kawaguchi
 */
 @Extension
-public class ListChangesCommand extends AbstractBuildRangeCommand {
+public class ListChangesCommand extends JobRangeCommand {


No external usages, but also binary incompatible

oleg-nenashev · 2017-05-07T21:01:20Z

core/src/main/java/hudson/cli/ReloadJobCommand.java

@@ -78,11 +79,10 @@ protected int run() throws Exception {
                }

                if(job == null) {
-                    // TODO: JENKINS-30785
-                    AbstractProject project = AbstractProject.findNearest(job_s);
+                    AbstractItem project = Items.findNearest(AbstractItem.class, job_s, jenkins);


Why not just item? Or maybe even TopLevelItem (who would reload other items anyway?)

reload is defined in AbstractItem.

oleg-nenashev · 2017-05-07T21:05:30Z

core/src/main/java/jenkins/scm/RunWithSCM.java

+ *
+ * @since FIXME
+ */
+public interface RunWithSCM<JobT extends Job<JobT, RunT> & Queue.Task,


I am not sure it is safe to use Queue.Task/Executable here. It may be possible to imagine other Run types, and it does not seem to be really required. 🐜 , will be bug if I find a real use-case for it

@abayer fyi

Ok, I can remove those.

And done in 914963c

oleg-nenashev · 2017-05-07T21:08:35Z

core/src/main/java/hudson/model/AbstractItem.java

        // TODO can this (and its pseudo-override in AbstractProject) share code with GenericItemOptionHandler, used for explicit CLICommand’s rather than CLIMethod’s?
        AbstractItem item = Jenkins.getInstance().getItemByFullName(name, AbstractItem.class);
        if (item==null) {
-            AbstractProject project = AbstractProject.findNearest(name);
+            AbstractItem project = Items.findNearest(AbstractItem.class, name, Jenkins.getInstance());


AbstractItem is required for displaying FullName in the message. I think it could be generalized even more to Item

This resolver is in AbstractItem specifically.

oleg-nenashev · 2017-05-07T21:11:48Z

core/src/main/java/hudson/model/AbstractProject.java

    public boolean supportsMakeDisabled() {
        return this instanceof TopLevelItem;
    }

+    // Seems to be used only by tests; do not bother pulling up.


Add VisibleForTests annotation?

Well, that is the only call I found from inside this repository. Did not look externally.

oleg-nenashev · 2017-05-07T21:13:36Z

core/src/main/java/hudson/model/Job.java

+                this.idx = idx;
+            }
+
+            Run<?, ?> getBuild() {


Comment upstream please. Anyway

this is not an API

we commonly use the term build generically to include any Run

Plus it'd theoretically be changing an API, since this got moved over from AbstractBuild.

oleg-nenashev · 2017-05-07T21:15:14Z

core/src/main/java/hudson/model/Job.java

+            for (SCM s : scmItem.getSCMs()) {
+                scmNames.add(s.getDescriptor().getDisplayName());
+            }
+            scmDisplayName = " " + Util.join(scmNames, ", ");


Could be optimized to StringBuilder, issue in the migrated code anyway, so unrelated to this PR

Comment in the subsumed PR, not here.

oleg-nenashev · 2017-05-07T21:25:46Z

core/src/main/java/jenkins/model/ParameterizedJobMixIn.java

     */
-    public interface ParameterizedJob extends hudson.model.Queue.Task, hudson.model.Item {
+    public interface ParameterizedJob<JobT extends Job<JobT, RunT> & ParameterizedJobMixIn.ParameterizedJob<JobT, RunT> & Queue.Task, RunT extends Run<JobT, RunT> & Queue.Executable> extends BuildableItem {


🐛 It may sound ridiculous, but the original interface didn't actually require this parameterized job to be buildable. Don't ask why anyone would need parameters then...

By extending BuildableItem you made these items Buildable, which may impact UI (there will be build button, option to run the job in CLI, etc.). It may cause regressions in Job types, which are not supposed to be buildable. It needs more investigation before applying

The only implementations are AbstractProject and WorkflowJob, both of which were already BuildableItem. Does not directly impact the UI anyway, since the build button etc. is added explicitly by project types.

Anyway please comment in the subsumed PR which defines this change, not here.

oleg-nenashev · 2017-05-07T21:27:58Z

core/src/main/java/jenkins/scm/RunWithSCM.java

+/**
+ * Allows a {@link Run} to provide {@link SCM}-related methods, such as providing changesets and culprits.
+ *
+ * @since FIXME


So, Pipeline won't implement this API immediately, right?

Please comment on the subsumed PR.

@oleg-nenashev Correct, my plan is to not have anything downstream from #2730 merged/released until this is in an LTS that's been in the wild for a month.

Sole bug was defined in #2866, not here.

… as restricted, not APIs.

…sabled-JENKINS-27299

oleg-nenashev · 2017-05-10T07:52:48Z

I cannot review it until upstream PRs are merged. It is not effective

…S-41527

jglick · 2017-05-10T21:11:49Z

Effective diff from #2866 is fairly small.

oleg-nenashev

I am not fully convinced that breaking binary compatibility of CLI commands is perfectly safe hence they were effectively a part of public API. OTOH the only usage is in MatrixProject test suite, which will be unlikely affected, will it?

Other possible approach would be to create deleteRuns command instead of deleteBuilds, but it may cause much confusion among users.

So, 🐝 from me though it would be great to get extra feedback from @jenkinsci/code-reviewers .

olivergondza · 2017-05-12T09:17:29Z

core/src/main/java/hudson/cli/DeleteBuildsCommand.java

 @Extension
-public class DeleteBuildsCommand extends AbstractBuildRangeCommand {
+public class DeleteBuildsCommand extends JobRangeCommand {


JobRange? It is range of runs ...

True, RunRangeCommand would be a better name.

jglick · 2017-05-12T13:49:15Z

MatrixProject test suite, which will be unlikely affected

Access restrictions are not checked in test sources.

…JobRangeCommand.

jglick · 2017-05-12T20:21:29Z

@reviewbybees done

oleg-nenashev · 2017-05-14T02:25:52Z

@jenkinsci/code-reviewers This change is more readable now. Are you fine with this incompatible change (no risk within jenkinsci org && no expected valid use-case)?

oleg-nenashev · 2017-05-19T06:12:15Z

I assume everybody excepting me is fine with this compat change.

daniel-beck and others added 26 commits January 2, 2017 20:30

[FIX JENKINS-30785] Generalize some CLI stuff to AbstractItem

5046c56

Offering default methods on ParameterizedJob.

2e27be7

Javadoc typo.

fbadce3

Cleaner use of default methods in ParameterizedJob.

626b876

Need to pick up jenkinsci/bridge-method-injector#15 to be able to build.

3139f23

Sketch of pulling disabled functionality into ParameterizedJob.

d71db0f

EnableJobCommandTest.groovy → EnableJobCommandTest.java, and replacin…

9318eef

…g deprecated Remoting-based CLI calls with CLICommandInvoker.

All CLI commands could be broken by a missing CLI.*.shortDescription …

b7e1d86

…key on just one!

Forgot to move CLI method short descriptions to new package.

41630e2

Needed a @CLIResolver for ParameterizedJob. Adding an OptionHandler w…

1fffbed

…hile we are here.

Trying to fix up access-modifier versions; started failing in CI toda…

cd95d2b

…y for unknown reasons.

Introduced <p:makeDisabled/> by analogy with <p:config-disableBuild/>.

137b8ec

Using new type bounds.

36a5301

Merge branch 'ParameterizedJobMixIn-defaults' into AbstractProject.di…

1c759d3

…sabled-JENKINS-27299

access-modifier 1.11 released.

59fb6e6

MatrixProject and MavenModuleSet both expect to have access to makeDi…

aeb9952

…sabled.jelly.

Trying to generalize some more.

bb617a6

Minor simplification.

1517179

[JENKINS-34716] Generalizing doPolling and schedulePolling.

265ae0a

isBuildable

b6459e1

Obsolete comment.

30c6674

Updated comments.

244e419

bridge-method-injector 1.17

82ad44a

Unfortunately AbstractProject.schedulePolling cannot delegate to SCMT…

d24bb16

…riggerItem.

Merge branch 'jenkins-24141' of https://github.com/abayer/jenkins int…

7be31a1

…o CLI-JENKINS-41527

Making delete-builds and list-changes commands work with Pipeline.

ab290b6

jglick added the work-in-progress The PR is under active development, not ready to the final review label May 5, 2017

jglick requested a review from abayer May 5, 2017 22:52

jglick mentioned this pull request May 5, 2017

Integration tests for CLI commands jenkinsci/workflow-job-plugin#49

Merged

[FIXED JENKINS-41527] Made console CLI command compatible with Pipeline.

d36a1c9

oleg-nenashev previously requested changes May 7, 2017

View reviewed changes

jglick added 6 commits May 8, 2017 16:15

Merge branch 'master' into CLI-JENKINS-41527

8a75156

@oleg-nenashev agreed it would be clearer to explicitly mark commands…

d5e83dd

… as restricted, not APIs.

Updated tests to match slight message changes.

220e152

bridge-method-injector 1.17

faaee4c

Merge branch 'master' into ParameterizedJobMixIn-defaults

8f49071

Merge branch 'ParameterizedJobMixIn-defaults' into AbstractProject.di…

0a00506

…sabled-JENKINS-27299

oleg-nenashev self-assigned this May 10, 2017

oleg-nenashev mentioned this pull request May 10, 2017

Offering default methods on ParameterizedJob #2864

Merged

jglick added 3 commits May 10, 2017 17:07

Merge branch 'master' into CLI-JENKINS-41527

6a38c31

Merge branch 'master' into AbstractProject.disabled-JENKINS-27299

f0b79a0

Merge branch 'AbstractProject.disabled-JENKINS-27299' into CLI-JENKIN…

eed093e

…S-41527

oleg-nenashev approved these changes May 12, 2017

View reviewed changes

olivergondza reviewed May 12, 2017

View reviewed changes

@olivergondza pointed out that RunRangeCommand is a better name than …

59223d1

…JobRangeCommand.

jglick added the needs-more-reviews Complex change, which would benefit from more eyes label May 12, 2017

abayer approved these changes May 12, 2017

View reviewed changes

jglick added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels May 12, 2017

Merge branch 'master' into CLI-JENKINS-41527

ec4e66e

oleg-nenashev merged commit 33afbcc into jenkinsci:master May 19, 2017

jglick deleted the CLI-JENKINS-41527 branch May 19, 2017 12:45

Make all CLI commands compatible with Pipeline where possible #2874

Make all CLI commands compatible with Pipeline where possible #2874

Conversation

jglick commented May 5, 2017 • edited Loading

Description

Changelog entries

Desired reviewers

ghost commented May 6, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented May 10, 2017

jglick commented May 10, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented May 12, 2017

jglick commented May 12, 2017

oleg-nenashev commented May 14, 2017

oleg-nenashev commented May 19, 2017

jglick commented May 5, 2017 •

edited

Loading