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

Make all CLI commands compatible with Pipeline where possible #2874

Merged
merged 40 commits into from May 19, 2017

Conversation

@jglick
Copy link
Member

jglick commented May 5, 2017

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

daniel-beck and others added some commits Jan 2, 2017

EnableJobCommandTest.groovy → EnableJobCommandTest.java, and replacin…
…g deprecated Remoting-based CLI calls with CLICommandInvoker.

@jglick jglick requested a review from abayer May 5, 2017

@oleg-nenashev
Copy link
Member

oleg-nenashev left a comment

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

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

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 7, 2017

Member

🐜 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.

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Author Member

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

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 8, 2017

Member

CLI command classes are not considered APIs

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

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

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 7, 2017

Member

No external usages, but also binary incompatible

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Author Member

Ditto.

@@ -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);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 7, 2017

Member

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

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Author Member

reload is defined in AbstractItem.

This comment has been minimized.

Copy link
@oleg-nenashev
*
* @since FIXME
*/
public interface RunWithSCM<JobT extends Job<JobT, RunT> & Queue.Task,

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 7, 2017

Member

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

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Author Member

Ditto.

This comment has been minimized.

Copy link
@oleg-nenashev

This comment has been minimized.

Copy link
@abayer

abayer May 8, 2017

Member

Ok, I can remove those.

This comment has been minimized.

Copy link
@abayer

abayer May 8, 2017

Member

And done in 914963c

// 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());

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 7, 2017

Member

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

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Author Member

This resolver is in AbstractItem specifically.

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

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

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 7, 2017

Member

Add VisibleForTests annotation?

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Author Member

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

this.idx = idx;
}

Run<?, ?> getBuild() {

This comment has been minimized.

Copy link
@oleg-nenashev

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Author Member

Comment upstream please. Anyway

  • this is not an API
  • we commonly use the term build generically to include any Run

This comment has been minimized.

Copy link
@abayer

abayer May 8, 2017

Member

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

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

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 7, 2017

Member

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

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Author Member

Comment in the subsumed PR, not here.

*/
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 {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 7, 2017

Member

🐛 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

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Author Member

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.

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Author Member

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

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

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 7, 2017

Member

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

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Author Member

Please comment on the subsumed PR.

This comment has been minimized.

Copy link
@abayer

abayer May 8, 2017

Member

@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.

@oleg-nenashev oleg-nenashev self-assigned this May 10, 2017

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented May 10, 2017

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

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented May 10, 2017

Effective diff from #2866 is fairly small.

@oleg-nenashev
Copy link
Member

oleg-nenashev left a comment

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 .

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

This comment has been minimized.

Copy link
@olivergondza

olivergondza May 12, 2017

Member

JobRange? It is range of runs ...

This comment has been minimized.

Copy link
@jglick

jglick May 12, 2017

Author Member

True, RunRangeCommand would be a better name.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented May 12, 2017

MatrixProject test suite, which will be unlikely affected

Access restrictions are not checked in test sources.

@abayer

abayer approved these changes May 12, 2017

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented May 12, 2017

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented May 14, 2017

@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

This comment has been minimized.

Copy link
Member

oleg-nenashev commented May 19, 2017

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

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

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details

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

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