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

Developer API: Add new HttpResponses#errorJSON() methods #3379

Merged
merged 1 commit into from Apr 4, 2018

Conversation

4 participants
@Wadeck
Contributor

Wadeck commented Apr 3, 2018

See #3082, comment from Devin asking for the missing methods in HttpResponses.

Changelog entries

  • RFE: Developer API: Add new HttpResponses#errorJSON() methods

Desired reviewers

@reviewbybees @dwnusbaum

@Wadeck Wadeck requested a review from dwnusbaum Apr 3, 2018

@Wadeck Wadeck referenced this pull request Apr 3, 2018

Merged

[JENKINS-31661] Check the root url (Admin monitor + Wizard) #3082

3 of 3 tasks complete
@dwnusbaum

Just for posterity, I requested this for consistency with okJSON since #3082 adds public static HttpResponse errorJSON(String, Map<?,?>).

@dwnusbaum dwnusbaum requested a review from kuisathaverat Apr 3, 2018

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Apr 4, 2018

Seems reasonable, although slightly weird with the other PR possibly getting merged into a different release. These will need to be synchronized.

@oleg-nenashev oleg-nenashev changed the title from Modification after comment from Devin in #3082 to Developer API: Add new HttpResponses#errorJSON() methods Apr 4, 2018

@oleg-nenashev oleg-nenashev merged commit 6526181 into jenkinsci:master Apr 4, 2018

2 checks passed

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

This comment has been minimized.

Member

dwnusbaum commented Apr 4, 2018

@oleg-nenashev The other PR is on-hold so maybe we need to revert this until next week? If it's fine to just roll this into the other PR then maybe we should do that to avoid confusion. Sorry!

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Apr 4, 2018

@dwnusbaum This PR introduces new API, and it's quite self-consistent. I do not see why we would need to block it till another one is reviewed. We can revert it of course

@dwnusbaum

This comment has been minimized.

Member

dwnusbaum commented Apr 4, 2018

@oleg-nenashev No strong opinion, this one is definitely self-contained as you said. The motivation for my earlier comment was just because the other PR adds another overload of errorJSON it might make sense to deliver them at the same time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment