Modified credentialsId to never return null #135
Conversation
| } | ||
|
|
||
| return ""; | ||
| } catch(NullPointerException npe) { |
There was a problem hiding this comment.
catch Throwable instead of NullPointerException
There was a problem hiding this comment.
Are we sure we want to catch everything, no matter what?
| if (token != null) { | ||
| return token.getToken(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Add a small sleep ... maybe 500 milliseconds
|
On Thu, May 18, 2017 at 10:29 AM, Ryan Murphy ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/main/java/io/fabric8/jenkins/openshiftsync/CredentialsUtils.java
<#135 (comment)>
:
> + OpenShiftToken token = CredentialsMatchers.firstOrNull(
+ CredentialsProvider.lookupCredentials(
+ OpenShiftToken.class,
+ Jenkins.getActiveInstance(),
+ ACL.SYSTEM,
+ Collections.<DomainRequirement>emptyList()
+ ),
+ CredentialsMatchers.withId(GlobalPluginConfiguration.get().getCredentialsId())
+ );
+
+ if (token != null) {
+ return token.getToken();
+ }
+
+ return "";
+ } catch(NullPointerException npe) {
Are we sure we want to catch everything, no matter what?
Yes. That said, per the scrum, if just making sure the getting does not
return null fixes the issue
and we don't need retry, than I'm good with just pulling the retry.
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#135 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADbadMiQG5WnMyCZkMdk8xBrd_z9WZ45ks5r7FXYgaJpZM4NfSq0>
.
|
|
@gabemontero per scrum, looks like the only big change is returning nonnull in the getter for credentialsId. ptal? |
|
Ah note the travis failure @oatmealraisin - |
| String credentialsId = GlobalPluginConfiguration.get().getCredentialsId(); | ||
| if(credentialsId == "") { | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
have you done any testing to confirm what value you get back when there is a credential configured?
|
before we merge this i want to know that if the user has configured a credential for the plugin, we're not losing it during the reload. |
|
@bparees Non-empty credentialId's are definitely preserved. Putting a print in the getter, we can see: The restart happens at |
|
cool, thanks @oatmealraisin |
|
@gabemontero gave lgtm and tests passed, so merging. |
Fixes #132
For getCredentialsId, we need to do a null check there. Somehow, the String is getting set to null, although not through its setter. This only happens when it is already emptystring.
@gabemontero @openshift/devex ptal?