Automation equals methods

[samrocketman]

• Add equals method to GithubAuthorizationStrategy
• Add equals method to GithubSecurityRealm
• GithubAuthorizationStrategy equals unit tests
• GithubSecurityRealm equals unit tests

Add equals methods to GithubAuthorizationStrategy and GithubSecurityRealm. This way they can be easily compared in configuration management scripts.

The following is a GithubSecurityRealm example using new equals method configuring via Jenkins Script Console. It is idempotent and only modifies the Jenkins configuration if it has changed.

import hudson.security.SecurityRealm
import org.jenkinsci.plugins.GithubSecurityRealm
String githubWebUri = 'https://github.com'
String githubApiUri = 'https://api.github.com'
String clientID = 'someid'
String clientSecret = 'somesecret'
String oauthScopes = 'read:org'
SecurityRealm github_realm = new GithubSecurityRealm(githubWebUri, githubApiUri, clientID, clientSecret, oauthScopes)
if(!github_realm.equals(Jenkins.instance.getSecurityRealm())) {
    Jenkins.instance.setSecurityRealm(github_realm)
    Jenkins.instance.save()
}

[jenkinsadmin]

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

[samrocketman]

Code review, please.

[samrocketman]

@cloudbeesci peer review, please.

[daniel-beck]

👎 You cannot use == to compare objects for equality in Java.

[samrocketman]

@daniel-beck is there a recommended way of comparing objects? For me, something like this would be powerful for idempotent changes in configuration management. I suppose I could offload this equals functionality into the configuration management itself to stay idempotent. Though, I'm curious to hear more about your thoughts. I'm still relatively new to developing in Java.

Is this by convention? Not comparing objects in this way I mean. Because technically equals() seems to successfully override.

[samrocketman]

@daniel-beck after further research I disagree with the following statement.

👎 You cannot use == to compare objects for equality in Java.

You can use == to compare objects for equality in Java. That's the entire premise of the language. Take String as an example. java.lang.String derives from java.lang.Object. String implements an equals() method. String is an Object that is comparing to another object for equality.

It matters what properties of an object are being used for comparison. In this case, I'm comparing properties that matter between two objects. For example, I'm using the following properties to determine equality between GithubSecurityRealm objects: githubWebUri, githubApiUri, clientID, clientSecret, and oauthScopes.

Without implementing an equals() method for the objects, it will remain useless for comparison. I think this is still a useful implementation.

[samrocketman]

cc @jhoblitt code review, please.

[daniel-beck]

To clarify, you want two different instances of java.lang.String with the exact same text content to result in false (i.e. not equal)? Because this is what you're getting.

System.out.println("foo".equals(new String("foo")) ? "true" : "false");
System.out.println("foo" == new String("foo") ? "true" : "false");

Output:

true
false

[samrocketman]

I want to objects to be considered equal if its properties are equal. This is akin to the following example.

https://github.com/kohsuke/github-api/blob/master/src/main/java/org/kohsuke/github/GHUser.java#L175-L182

The equals() method is used by ==, correct?

[samrocketman]

@daniel-beck the following both output true in the Groovy script console of Jenkins.

println "foo".equals(new String("foo")) ? "true" : "false"
println "foo" == new String("foo") ? "true" : "false"

[daniel-beck]

Yes, because Groovy isn't Java. This code is written in Java though.

[samrocketman]

But as you can see, I'm using .equals in the Java unit tests and not using ==. I guess I'm not really seeing the issue or perhaps a better workaround if .equals() method is not the proper way to do it. Can you provide an alternative? .equals() seems to be a typical use case. Unless you mean I should update all of the checks internally when comparing the objects to .equals()? Can you be more clear with your feedback?

Functions like this.isUseRepositoryPermissions() == obj.isUseRepositoryPermissions() are comparing Booleans. And functions like this.getOrganizationNames() == obj.getOrganizationNames() are comparing Strings. Are you suggesting that Booleans and Strings shouldn't be compared in this manner and should use .equals() instead of ==?

[daniel-beck]

And functions like this.getOrganizationNames() == obj.getOrganizationNames() are comparing Strings. Are you suggesting that … Strings shouldn't be compared in this manner and should use .equals() instead of ==?

This is exactly what I'm saying. Hence my first comment (strings are objects in Java), and the demo program I provided that shows you why you should not use == on strings.

[samrocketman]

I tried doing .equals with a boolean (e.g. this.isUseRepositoryPermissions().equals(obj.isUseRepositoryPermissions())) I got an error: compilation error boolean cannot be dereferenced.

I've looked it up and turns out I can't do .equals() with a boolean because it is a primitive. However, I was able to do it to the String objects. I'm going to update the commit.

[samrocketman]

@daniel-beck I have pushed an updated commit with your feedback. Thanks for your patience.

[samrocketman]

@daniel-beck can I get a 👍 ?

[daniel-beck]

Superfluous surrounding parentheses.

[daniel-beck]

I'm not familiar with this plugin, so I have no idea regarding the larger ideas behind this PR. That prevents me from providing 👍, however, I can say that there are only minor nitpicks left from my POV.

[samrocketman]

Updated the pull request with the latest feedback. As soon as it verifies good I'll merge it.