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-28071] generalise CLI's BuildCommand target from AbstractProject to Job #1979

Merged
merged 3 commits into from May 14, 2016

Conversation

@corngood
Copy link
Contributor

corngood commented Jan 15, 2016

JENKINS-28071

This allows e.g. WorkflowJob to build, fixing symptom JENKINS-29826

JobOptionHandler is based on AbstractProjectOptionHandler. I'm not sure if the latter is still needed. The absence of JobOptionHandler only caused a runtime error, so I'm reluctant to remove it.

I didn't yet put any effort into abstracting (or finding an abstraction for) isDisabled or poll.

TODO: tests

/*
* The MIT License
*
* Copyright (c) 2004-2009, Sun Microsystems, Inc.

This comment has been minimized.

Copy link
@KostyaSha

This comment has been minimized.

Copy link
@corngood

corngood Jan 15, 2016

Author Contributor

This is based on a file with this license and (c). What do you expect here?

This comment has been minimized.

Copy link
@jtnord

jtnord Jan 15, 2016

Member

as this is a new file the date should be the current year - and the copyright owner should be who owns the copyright of this code (either you or the company you work for depending on when you did this code change and your employment contract)

e..g
Copyright (c) 2016, David McFarland or Copyright (c) 2016, Acme Inc. (if you work for Acme Inc,)

This comment has been minimized.

Copy link
@corngood

corngood Jan 15, 2016

Author Contributor

"The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software."

Since this file is essentially a rename I thought it was appropriate to leave it intact (the dates were never changed in other revisions).

If I had written it from scratch I never would have put a copyright/license on it at all (as in TopLevelItemOptionHandler.java). I can take it off completely if that's better.

This comment has been minimized.

Copy link
@jtnord

jtnord Jan 15, 2016

Member

Ok I missed this was a a rename.
Probably should have the date range updated and you added as a contributor, but I think if this was just a rename then we are ok

This comment has been minimized.

Copy link
@corngood

corngood Jan 22, 2016

Author Contributor

I didn't bother doing that since nobody else has when they worked on the source file... Doesn't matter to me though.

@corngood corngood changed the title [JENKINS-28071] WIP: generalise BuildCommand target from AbstractProject to Job [JENKINS-28071] generalise BuildCommand target from AbstractProject to Job Jan 22, 2016
@corngood

This comment has been minimized.

Copy link
Contributor Author

corngood commented Jan 22, 2016

We've been running this for a while with no problems, and I haven't had any comments on the implementation, so removing WIP. Do I need to do anything to remove the tag?

@corngood

This comment has been minimized.

Copy link
Contributor Author

corngood commented Feb 1, 2016

@oleg-nenashev Should I reopen to remove the WIP flag? This is ready to merge IMO.

@corngood

This comment has been minimized.

Copy link
Contributor Author

corngood commented Feb 3, 2016

@jglick Could I get your input on this, since it's meant to allow CLI builds of workflows?

if (checkSCM) {
if (job.poll(new StreamTaskListener(stdout, getClientCharset())).change == Change.NONE) {
if (checkSCM && job instanceof AbstractProject<?, ?>) {
if (((AbstractProject<?, ?>)job).poll(new StreamTaskListener(stdout, getClientCharset())).change == Change.NONE) {

This comment has been minimized.

Copy link
@amuniz

amuniz Feb 4, 2016

Member

You can run polling in general by using SCMTriggerItems.asSCMTriggerItem(job).poll(listener).hasChanges()

This comment has been minimized.

Copy link
@jglick

jglick Feb 23, 2016

Member

Right, this needs to be fixed.

This comment has been minimized.

Copy link
@jglick

jglick Feb 23, 2016

Member

For example

if (checkSCM) {
    SCMTriggerItem pollable = SCMTriggerItems.asSCMTriggerItem(job);
    if (pollable != null && pollable.poll(new StreamTaskListener(stdout, getClientCharset())).change == Change.NONE) {

(I suspect the original code ought to have been using hasChanges() to deal with INSIGNIFICANT, but anyway that is outside the scope of this PR.)

@amuniz

This comment has been minimized.

Copy link
Member

amuniz commented Feb 4, 2016

LGTM 👍

@amuniz

This comment has been minimized.

Copy link
Member

amuniz commented Feb 23, 2016

Is something preventing to merge this?
@oleg-nenashev do you think it's still in-progress?

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Feb 23, 2016

First I have heard of it, I can take a look.

return 0;
}
}

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

This comment has been minimized.

Copy link
@jglick

jglick Feb 23, 2016

Member

JENKINS-27299 for reference, but anyway this only comes down to the choice of a message.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 11, 2016

Member

Maybe it's a subject for the core enhancement. I see no reason why other Job types have no such method in API

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Feb 23, 2016

JobOptionHandler is based on AbstractProjectOptionHandler. I'm not sure if the latter is still needed.

I think so, in case other commands continue to request an AbstractProject specifically.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Feb 23, 2016

Still in progress until the poll call is fixed, I suppose.

…o Job

This allows e.g. WorkflowJob to build, fixing symptom [JENKINS-29826]
@corngood corngood force-pushed the corngood:cli_build_jobs branch from e3bc3b3 to e889dc5 Feb 23, 2016
@corngood

This comment has been minimized.

Copy link
Contributor Author

corngood commented Feb 23, 2016

Updated with poll suggestion. This compiles, but I haven't tested it myself.

@amuniz

This comment has been minimized.

Copy link
Member

amuniz commented Feb 25, 2016

Tests are passing, so LGTM - again - 👍

@corngood

This comment has been minimized.

Copy link
Contributor Author

corngood commented Mar 11, 2016

Bump. Who do I need to merge this?

@corngood corngood closed this Apr 14, 2016
@corngood corngood reopened this Apr 14, 2016
@corngood

This comment has been minimized.

Copy link
Contributor Author

corngood commented Apr 14, 2016

@jglick Can you help me out with this? Seems to be in limbo.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented May 11, 2016

Fixed the label. Just for the future: please do not squash before the review finishes. It does not allow reviewing diffs after comments

@@ -135,14 +142,14 @@ protected int run() throws Exception {
}

if (checkSCM) {
if (job.poll(new StreamTaskListener(stdout, getClientCharset())).change == Change.NONE) {
if (!SCMTriggerItem.SCMTriggerItems.asSCMTriggerItem(job).poll(new StreamTaskListener(stdout, getClientCharset())).hasChanges()) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 11, 2016

Member

asSCMTriggerItem(job) may return null, needs check

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented May 11, 2016

Reverted my +1. There is a potential NPE

@corngood

This comment has been minimized.

Copy link
Contributor Author

corngood commented May 11, 2016

@oleg-nenashev Like so?

Let me know if you want me to squash it.

@@ -135,14 +142,15 @@ protected int run() throws Exception {
}

if (checkSCM) {
if (job.poll(new StreamTaskListener(stdout, getClientCharset())).change == Change.NONE) {
SCMTriggerItem item = SCMTriggerItem.SCMTriggerItems.asSCMTriggerItem(job);
if (item != null && !item.poll(new StreamTaskListener(stdout, getClientCharset())).hasChanges()) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 11, 2016

Member

If it is null, we cannot check SCM. Maybe makes sense to fail the command in such case

@corngood

This comment has been minimized.

Copy link
Contributor Author

corngood commented May 12, 2016

@oleg-nenashev Now with exception for missing SCM trigger

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented May 12, 2016

👍

@corngood

This comment has been minimized.

Copy link
Contributor Author

corngood commented May 13, 2016

@oleg-nenashev Should I squash it? Is there anything else I can do to help get this merged?

Cheers

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented May 13, 2016

@corngood Everything is fine, it's possible to squash via GitHub web UI now. I'm just waiting for the feedback from somebody, we commonly merge after two+ votes or after a timeout. If there is no responses, I'll merge it tomorrow.

CC @amuniz @jglick @jtnord

@oleg-nenashev oleg-nenashev changed the title [JENKINS-28071] generalise BuildCommand target from AbstractProject to Job [JENKINS-28071] generalise CLI's BuildCommand target from AbstractProject to Job May 14, 2016
@oleg-nenashev oleg-nenashev merged commit 6ceccc0 into jenkinsci:master May 14, 2016
1 check passed
1 check passed
Jenkins This pull request looks good
Details
@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented May 14, 2016

@corngood
Thanks for your patience! It's integrated now

oleg-nenashev added a commit that referenced this pull request May 15, 2016
@corngood corngood deleted the corngood:cli_build_jobs branch May 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.