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.

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

JBIDE-20719 Updating a token for OpenShift 3 connection via Edit Connection does not update resources #951

Closed
wants to merge 1 commit into from

Conversation

scabanovich
Copy link
Contributor

No description provided.

return false;
}
return true;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Having a special #equals for credentials seems legit since we consider 2 connections as identical if they only differ in authentication means (ex. password or token). On the other hand the username is used to persist them and thus is required to identify and #equal 2 connections. I thus dont think that #equals may be true while Objects.equals(getUsername(), other.getUsername()) returns false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I moved comparing userName to eduals().

@scabanovich scabanovich force-pushed the jbide-20719 branch 2 times, most recently from 216382d to 0b235f1 Compare February 5, 2016 15:24
//It is safe to cast now.
Connection other = (Connection)connection;
//User name is already compared
if(!Objects.equals(password, other.password)) {
Copy link
Member

Choose a reason for hiding this comment

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

I assume that a purely token based connection will always have null in the password?
I admit that this part is rather fuzzy in our current Connection implementation, we have the authentication strategies but then we still have the password and username in connection. We should clean this stuff up at some point I think

@adietish
Copy link
Member

adietish commented Feb 5, 2016

looks good, I couldnt test it, but if you tested it (in the UI, too - i saw the unit tests :) ) then I'd +1

@scabanovich
Copy link
Contributor Author

@adietish , unfortunately I cannot test it. One need to have token, and have it expired. May we ask @mlabuda to test it?

@fbricon
Copy link
Member

fbricon commented Feb 5, 2016

I tested it works, with both username change and token update
+1

@fbricon
Copy link
Member

fbricon commented Feb 5, 2016

Applied in master / 4.3.x. Thanks @scabanovich !

@fbricon fbricon closed this Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants