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

[FIXED JENKINS-37590] Return a null property if no parameters #2723

Merged
merged 2 commits into from
Feb 9, 2017
Merged

[FIXED JENKINS-37590] Return a null property if no parameters #2723

merged 2 commits into from
Feb 9, 2017

Conversation

abayer
Copy link
Member

@abayer abayer commented Jan 20, 2017

JENKINS-37590

At least part of the impetus for this issue was dealt with a while
back by a revert of PR #2444, but this gives us a better solution
anyway by guaranteeing we have a null property rather than one without
parameters.

cc @reviewbybees esp @jglick

@abayer
[FIXED JENKINS-37590] Return a null property if no parameters
8a8c4c2 
At least part of the impetus for this issue was dealt with a while
back by a revert of PR jenkinsci#2444, but this gives us a better solution
anyway by guaranteeing we have a null property rather than one without
parameters.
@ghost
Copy link

ghost commented Jan 20, 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

Doesn't this completely negate the advantage OptionalJobPropertyDescriptor was supposed to bring?

@rsandell
Copy link
Member

🐝

@jglick
Copy link
Member

jglick commented Jan 30, 2017

Doesn't this completely negate the advantage OptionalJobPropertyDescriptor was supposed to bring?

Only partially. The super implementation and the Jelly views are still useful.

jglick
jglick approved these changes Jan 30, 2017
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Good but reverting the now-obsolete #2444 would be better.

@jglick
Copy link
Member

jglick commented Jan 30, 2017

Specifically #2660 seems to have partially reverted it, but the two (redundant?!) patches to Messages.properties were forgotten.

@abayer
Copy link
Member Author

abayer commented Jan 30, 2017

Aaaah, I'll get the Messages.properties changes reverted in here too.

oleg-nenashev
oleg-nenashev approved these changes Jan 30, 2017
View reviewed changes
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.

LGTM 🐝

@jglick jglick added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Feb 6, 2017
@daniel-beck daniel-beck merged commit 6d51a5f into jenkinsci:master Feb 9, 2017
daniel-beck added a commit that referenced this pull request Feb 13, 2017
@daniel-beck
Noting #2722 #2723 #2727 #2732 #2735 #2737 #2738 #2739 #2744
f6005c3
@oleg-nenashev
Copy link
Member

This fix actually didn't fix the reported issue (though it fixed others): https://issues.jenkins-ci.org/browse/JENKINS-46638

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

@jglick jglick jglick approved these changes

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

Assignees

@jglick jglick

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@abayer @daniel-beck @rsandell @jglick @oleg-nenashev