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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-36193] Raise exception when trying to build a job that is disabled #2432

Merged
merged 1 commit into from Jul 16, 2016
Merged

[JENKINS-36193] Raise exception when trying to build a job that is disabled #2432

merged 1 commit into from Jul 16, 2016

Conversation

godfath3r
Copy link
Contributor

@godfath3r godfath3r commented Jul 3, 2016 •
edited by jglick

@godfath3r godfath3r mentioned this pull request Jul 3, 2016
Closed
@jtnord
Copy link
Member

jtnord commented Jul 4, 2016

👍

@oleg-nenashev
Copy link
Member

The test failure is unrelated, retriggering just in case.
But 👍

@oleg-nenashev oleg-nenashev reopened this Jul 4, 2016
@oleg-nenashev oleg-nenashev changed the title [JENKINS-36193] Raise exception when trying to build a job that is di… [JENKINS-36193] Raise exception when trying to build a job that is disabled Jul 4, 2016
@oleg-nenashev
Copy link
Member

Retriggering build after #2435

@oleg-nenashev oleg-nenashev reopened this Jul 5, 2016
@oleg-nenashev oleg-nenashev added needs-testcase Test automation is required for this pull request ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback labels Jul 6, 2016
@oleg-nenashev
Copy link
Member

Ready for merge. I'll wait till Friday in the case @godfath3r finds some time to write tests

jglick
jglick reviewed Jul 8, 2016
View reviewed changes
core/src/main/java/jenkins/model/ParameterizedJobMixIn.java
@@ -178,7 +178,7 @@ public final void doBuild(StaplerRequest req, StaplerResponse rsp, @QueryParamet
}

if (!asJob().isBuildable()) {
throw HttpResponses.error(SC_INTERNAL_SERVER_ERROR, new IOException(asJob().getFullName() + " is not buildable"));
throw HttpResponses.error(SC_CONFLICT, new IOException(asJob().getFullName() + " is not buildable"));
Copy link
Member

Choose a reason for hiding this comment

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

Somewhat creative use of a 409, but as good a status as any I suppose.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW build-token-root currently uses a 400.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

400 Bad Request The request could not be understood by the server due to malformed syntax. The client SHOULD NOT repeat the request without modifications.

409 Conflict The request could not be completed due to a conflict with the current state of the resource. This code is only allowed in situations where it is expected that the user might be able to resolve the conflict and resubmit the request. The response body SHOULD include enough

Taken from here: https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html

409 seems more close to the situation. What do you guys say? If SC_CONFLICT seems better then, build-token-root should also use 409.

@jglick
Copy link
Member

jglick commented Jul 8, 2016

Definitely could use a new test demonstrating the bug. This seems to be the only existing test in Jenkins core covering a related topic, which is alarming.

@godfath3r
Copy link
Contributor Author

I will try to write some test. Please don't merge until then.

@oleg-nenashev oleg-nenashev added work-in-progress The PR is under active development, not ready to the final review and removed ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback labels Jul 9, 2016
@oleg-nenashev
Copy link
Member

I'll write the test in a follow-up pull request

@oleg-nenashev oleg-nenashev removed needs-testcase Test automation is required for this pull request work-in-progress The PR is under active development, not ready to the final review labels Jul 15, 2016
@oleg-nenashev
Copy link
Member

We had a discussion with @godfath3r in IRC about it several days ago

@oleg-nenashev oleg-nenashev added the work-in-progress The PR is under active development, not ready to the final review label Jul 15, 2016
@oleg-nenashev oleg-nenashev mentioned this pull request Jul 15, 2016
Merged
@oleg-nenashev oleg-nenashev merged commit ab26fdb into jenkinsci:master Jul 16, 2016
@oleg-nenashev
Copy link
Member

Merged as a part of #2457

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
work-in-progress The PR is under active development, not ready to the final review
Projects
None yet
Milestone
No milestone
4 participants
@godfath3r @jtnord @oleg-nenashev @jglick