[JBIDE-15766]:Remove refresh of all IEnvironmentVariable(s) on add/remove actions #100
Conversation
There was a problem hiding this comment.
I would suggest keeping the "ByName" since its more explicit than the "Map", it tells you what the keys and what the values are.
There was a problem hiding this comment.
Looks like I am going to have to rebase and then update the PR again.
I see that I got a message from OpenShift-bot saying that the PR can't be automerged.
Respectfully,
Martes G Wigglesworth
Consultant - Middleware Engineer
Red Hat Consulting
Red Hat, Inc.
----- Original Message -----
From: "André Dietisheim" notifications@github.com
To: "openshift/openshift-java-client" openshift-java-client@noreply.github.com
Cc: "Martes Wigglesworth" mwigglesworth@redhat.com
Sent: Friday, November 8, 2013 12:01:28 PM
Subject: Re: [openshift-java-client] [JBIDE-15766]:Remove refresh of all IEnvironmentVariable(s) on add/remove actions (#100)
In src/main/java/com/openshift/internal/client/ApplicationResource.java:
@@ -149,7 +151,7 @@
/**
- The environment variables for this application
*/
- private Map<String, IEnvironmentVariable> environmentVariableByName;
- private Map<String, IEnvironmentVariable> environmentVariablesMap;
I would suggest keeping the "ByName" since its more explicit than the "Map", it tells you what the keys and what the values are.
—
Reply to this email directly or view it on GitHub .
|
martes: as you can see in this pull-request: "We can’t automatically merge this pull request.". You need to rebase it and push it again. |
|
Yeah, I know. I just saw the message about an hour ago. I am working on it now. Respectfully, Martes G Wigglesworth ----- Original Message ----- From: "André Dietisheim" notifications@github.com martes: as you can see in this pull-request: "We can’t automatically merge this pull request.". You need to rebase it and push it again. — |
There was a problem hiding this comment.
Unreachable code. It's already been checked for null and an exception was thrown. It cannot possibly be null now
Edit: Oh, its commented. Confusing to read
…e actions Please reference the following change log: Removed update calls from EnvironmentVariableResource.destroy, ApplicationResource.addEnvironmentVariable,ApplicationResource.addEnvironmentVariables,and ApplicationResource.removeEnvironmentVariable. Added ApplicationResource.removeEnvironmentVariable(IEnvironmentVariable) Renamed environmentVariableByName to environmentVariableMap Updated getOrLoadEnvironmentVariable to work check for emptiness of environmentVariableMap Updated loadEnvironmentVariables to work on environmentVariablesMap, and return it, in contrast to a locally-scoped version that did not persist. Corrected test failures due to omitted changes to instances methods for EnvironmentVariableResouce and ApplicationResource. Updated remove methods to use remove..(IEnvironmentVariable) and ApplicationResourceIntegrationTest to use Application.removeEnvironmentVariable(IEnvironmentVariable) in testing. Added integration test for removeEnvironmentVariable(IEnvironmentVariable) Updated ApplicationResource.addEnvironmentVariables(Map<String,String>) to filter the incoming map key/value pairs if they are duplicate keys, without different values. This update should resolve unecessary update DTO requests being generated. (Rebased up to Nov 12 since the original PR got behind.) Corrected possible logical error that was accidentally included in addEnvinronmentVariables(Map<String,String>), with the original PR. [if(testVar!=null)...else....] (Rebased again, to support merge with most recent version of upstream master: 13-NOV-2013) Corrected ApplicationResource.removeEnvironmentVariableByInstance(IEnvironmentVariable) and ApplicationResource.removeEnvironmentVariable(String) to correct failing test(s).
Please reference the following change log:
Removed update calls from EnvironmentVariableResource.destroy,
ApplicationResource.addEnvironmentVariable,ApplicationResource.addEnvironmentVariables,and
ApplicationResource.removeEnvironmentVariable.
Added
ApplicationResource.removeEnvironmentVariable(IEnvironmentVariable)
Resolves #9
Renamed environmentVariableByName to environmentVariableMap
Updated getOrLoadEnvironmentVariable to work check for emptiness of
environmentVariableMap
Updated loadEnvironmentVariables to work on environmentVariablesMap, and
return it, in contrast to a locally-scoped version that did not persist.
Removed update calls from EnvironmentVariableResource.destroy,
ApplicationResource.addEnvironmentVariable,ApplicationResource.addEnvironmentVariables,and
ApplicationResource.removeEnvironmentVariable.
Added
ApplicationResource.removeEnvironmentVariable(IEnvironmentVariable)
Renamed environmentVariableByName to environmentVariableMap
Updated getOrLoadEnvironmentVariable to work check for emptiness of
environmentVariableMap
Updated loadEnvironmentVariables to work on environmentVariablesMap, and
return it, in contrast to a locally-scoped version that did not persist.
Corrected test failures due to omitted changes to instances methods for EnvironmentVariableResouce and ApplicationResource.
Updated remove methods to use remove..(IEnvironmentVariable) and
ApplicationResourceIntegrationTest to use
Application.removeEnvironmentVariable(IEnvironmentVariable) in testing.
Added integration test for
removeEnvironmentVariable(IEnvironmentVariable)