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-27363,JENKINS-27665] - Annotate sensibleVariables (may be null) #4

Merged
merged 1 commit into from Jul 30, 2015

Conversation

Projects
None yet
6 participants
@oleg-nenashev
Member

oleg-nenashev commented Mar 11, 2015

Patch to the core: jenkinsci/jenkins#1601

Signed-off-by: Oleg Nenashev o.v.nenashev@gmail.com

[JENKINS-27363] - Annotate sensibleVariables (may be null)
Patch to the core: jenkinsci/jenkins#1601

Signed-off-by: Oleg Nenashev <o.v.nenashev@gmail.com>
@jenkinsadmin

This comment has been minimized.

Show comment
Hide comment
@jenkinsadmin

jenkinsadmin Mar 11, 2015

Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

Member

jenkinsadmin commented Mar 11, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@@ -155,7 +160,7 @@ public Object getTarget() {
throw new UnsupportedOperationException();
}
public Set<String> getSensibleVariables() {
public @CheckForNull Set<String> getSensibleVariables() {

This comment has been minimized.

@wolfs

wolfs Mar 11, 2015

Member

Shouldn't this method return an empty Set when sensibleVariables is null?

@wolfs

wolfs Mar 11, 2015

Member

Shouldn't this method return an empty Set when sensibleVariables is null?

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Mar 11, 2015

Member

It makes sense for the particular class, but unfortunately it may be overridden by child classes from external plugins (similarly to the core's method in jenkinsci/jenkins#1601).

Since the method belongs to a public library with unknown number of users, I'm not sure that making a strict annotation is a right choice now.

@oleg-nenashev

oleg-nenashev Mar 11, 2015

Member

It makes sense for the particular class, but unfortunately it may be overridden by child classes from external plugins (similarly to the core's method in jenkinsci/jenkins#1601).

Since the method belongs to a public library with unknown number of users, I'm not sure that making a strict annotation is a right choice now.

This comment has been minimized.

@wolfs

wolfs Mar 11, 2015

Member

The method should probably be final...

@wolfs

wolfs Mar 11, 2015

Member

The method should probably be final...

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jul 9, 2015

Member

No way back without releasing a major version with a formal "backward incompativility"

@oleg-nenashev

oleg-nenashev Jul 9, 2015

Member

No way back without releasing a major version with a formal "backward incompativility"

@aheritier

This comment has been minimized.

Show comment
Hide comment
@aheritier

aheritier Jul 9, 2015

Member

@wolfs @gboissinot @reviewbybees could you give a look at it ? This issue occurs frequently for users of the promoted-build plugin if they use it on a build launched in a previous launch of the Jenkins server.

Cheers

Member

aheritier commented Jul 9, 2015

@wolfs @gboissinot @reviewbybees could you give a look at it ? This issue occurs frequently for users of the promoted-build plugin if they use it on a build launched in a previous launch of the Jenkins server.

Cheers

@aheritier

This comment has been minimized.

Show comment
Hide comment
@aheritier

aheritier Jul 9, 2015

Member

🐝

Member

aheritier commented Jul 9, 2015

🐝

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Jul 9, 2015

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.

reviewbybees commented Jul 9, 2015

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.

@amuniz

This comment has been minimized.

Show comment
Hide comment
@amuniz

amuniz commented Jul 13, 2015

🐝

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Jul 13, 2015

Member

@wolfs @gboissinot
Would you mind if I go forward and merge/release this change?

@reviewbybees done

Member

oleg-nenashev commented Jul 13, 2015

@wolfs @gboissinot
Would you mind if I go forward and merge/release this change?

@reviewbybees done

@aheritier

This comment has been minimized.

Show comment
Hide comment
@aheritier

aheritier Jul 30, 2015

Member

@oleg-nenashev @daniel-beck sadly it seems we won't have a feedback from @wolfs and @gboissinot
Can we merge/release it ?

Member

aheritier commented Jul 30, 2015

@oleg-nenashev @daniel-beck sadly it seems we won't have a feedback from @wolfs and @gboissinot
Can we merge/release it ?

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Jul 30, 2015

Member

I'll merge and release it on this evening if there's no response. I've pinged the owners two weeks ago, so I can wear the core team member's hat for a while

Member

oleg-nenashev commented Jul 30, 2015

I'll merge and release it on this evening if there's no response. I've pinged the owners two weeks ago, so I can wear the core team member's hat for a while

@aheritier

This comment has been minimized.

Show comment
Hide comment
@aheritier
Member

aheritier commented Jul 30, 2015

oleg-nenashev added a commit that referenced this pull request Jul 30, 2015

Merge pull request #4 from oleg-nenashev/JENKINS-27363
[JENKINS-27363,JENKINS-27665] - Annotate sensibleVariables (may be null)

@oleg-nenashev oleg-nenashev merged commit 8e3c2bb into jenkinsci:master Jul 30, 2015

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