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

[JENKINS-27299] Define disabled in ParameterizedJob rather than AbstractProject #2866

Merged

Conversation

jglick
Copy link
Member

@jglick jglick commented May 3, 2017

JENKINS-27299

Subsumes #2864. Supersedes #2544.

@reviewbybees

@jglick jglick added the work-in-progress The PR is under active development, not ready to the final review label May 3, 2017
void test() {
def p = j.createFreeStyleProject()

def cli = new CLI(j.getURL())
Copy link
Member Author

Choose a reason for hiding this comment

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

One of a few forgotten in #2795 because they were in Groovy code and so did not appear in my usage search.

@jglick jglick added needs-more-reviews Complex change, which would benefit from more eyes and removed work-in-progress The PR is under active development, not ready to the final review labels May 5, 2017
@jglick jglick requested a review from abayer May 5, 2017 19:28
@ghost
Copy link

ghost commented May 5, 2017

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.

@daniel-beck
Copy link
Member

Unfortunately AbstractProject.schedulePolling cannot delegate to SCMTriggerItem.

Why?

@jglick
Copy link
Member Author

jglick commented May 5, 2017

Why?

Because it cannot call this.schedulePolling(), lest we go into an infinite loop (as FindBugs was so kind as to point out); and SCMTriggerItem.super.schedulePolling() will not compile since AbstractProject itself is not an SCMTriggerItem (Project is).

@abayer
Copy link
Member

abayer commented May 9, 2017

I really can't review this effectively until #2864 gets merged.

@jglick
Copy link
Member Author

jglick commented May 9, 2017

I really can't review this effectively until #2864 gets merged.

Would an effective diff link help? You cannot comment on it but you can see the new changes, and refer back to files of this PR to add comments.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

🐝 from what I see

@@ -158,7 +158,7 @@ protected int run() throws Exception {

if (!job.isBuildable()) {
String msg = Messages.BuildCommand_CLICause_CannotBuildUnknownReasons(job.getFullDisplayName());
if (job instanceof AbstractProject<?, ?> && ((AbstractProject<?, ?>)job).isDisabled()) {
if (job instanceof ParameterizedJobMixIn.ParameterizedJob && ((ParameterizedJobMixIn.ParameterizedJob) job).isDisabled()) {
Copy link
Member

Choose a reason for hiding this comment

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

Not 100% clear. Would be preferable to also have it on the Job level directly or in BuildableItem. But better than it was before

Copy link
Member Author

Choose a reason for hiding this comment

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

It is indeed a little tricky. Job does define buildable (without any explanation), but the idea here was to define disabled in ParameterizedJob since that is now the home for “Job which we could potentially request a build of, perhaps with parameters, so long as that is not disabled” (i.e., basically everything except ExternalJob).

BuildableItem (not to be confused with Queue.BuildableItem!), well, this interface is a historical relic: very little code actually deals with objects declared to be of this type and expects scheduleBuild overloads to work. I am implementing it for completeness; AbstractProject and WorkflowJob, the only ParameterizedJobs out there, already implemented it anyway, so this is just to make it easier to provide default implementations for those four methods (mostly superseded by scheduleBuild2 overloads).

@@ -279,7 +280,7 @@ protected int run() throws Exception {
throw new UnsupportedOperationException();
}
}));
} catch (ClassNotFoundException e) {
} catch (ClassNotFoundException | MissingResourceException e) {
Copy link
Member

Choose a reason for hiding this comment

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

huh? is it related?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, see commit introducing it. The enable-job and disable-job @CLIMethods were moved, and if you forget to move the corresponding shortDescription, as I initially did, you not only break those CLI commands, you break every other CLI command too.

Copy link
Member

Choose a reason for hiding this comment

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

agreed

@jglick
Copy link
Member Author

jglick commented May 11, 2017

@reviewbybees done

@jenkinsci/code-reviewers may be interested in this & related PRs

@jglick 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 11, 2017
@oleg-nenashev
Copy link
Member

Occasionally merged upstream PR with squash. Will cleanup and merge other stuff towards 2.61 as we discussed at the governance meeting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants