-
Notifications
You must be signed in to change notification settings - Fork 66
[JBIDE-15766]:Remove refresh of all IEnvironmentVariable(s) on add/remove actions #100
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,10 +67,12 @@ | |
import com.openshift.internal.client.utils.StringUtils; | ||
|
||
/** | ||
* The Class Application. | ||
* The ApplicationResource object is an implementation of com.openshift.client.IApplication, and provides | ||
* a runtime model for the real application that resides on the OpenShift platform being accessed. | ||
* | ||
* @author André Dietisheim | ||
* @author Syed Iqbal | ||
* @author Martes G Wigglesworth | ||
*/ | ||
public class ApplicationResource extends AbstractOpenShiftResource implements IApplication { | ||
|
||
|
@@ -149,7 +151,7 @@ public class ApplicationResource extends AbstractOpenShiftResource implements IA | |
/** | ||
* The environment variables for this application | ||
*/ | ||
private Map<String, IEnvironmentVariable> environmentVariableByName; | ||
private Map<String, IEnvironmentVariable> environmentVariablesMap; | ||
|
||
|
||
protected ApplicationResource(ApplicationResourceDTO dto, DomainResource domain) { | ||
|
@@ -203,6 +205,7 @@ protected ApplicationResource(final String name, final String uuid, final String | |
this.domain = domain; | ||
this.aliases = aliases; | ||
updateCartridges(cartridgesByName); | ||
environmentVariablesMap = new HashMap<String, IEnvironmentVariable>(); | ||
} | ||
|
||
public String getName() { | ||
|
@@ -611,11 +614,11 @@ public Map<String, IEnvironmentVariable> getEnvironmentVariables() throws OpenSh | |
return Collections.unmodifiableMap(new LinkedHashMap<String, IEnvironmentVariable>(getOrLoadEnvironmentVariables())); | ||
} | ||
|
||
protected Map<String, IEnvironmentVariable> getOrLoadEnvironmentVariables() throws OpenShiftException { | ||
if (environmentVariableByName == null) { | ||
this.environmentVariableByName = loadEnvironmentVariables(); | ||
} | ||
return environmentVariableByName; | ||
|
||
protected Map<String, IEnvironmentVariable> getOrLoadEnvironmentVariables() throws OpenShiftException { | ||
if(environmentVariablesMap.isEmpty()) | ||
environmentVariablesMap = loadEnvironmentVariables(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there are no env vars in the applciation in the backend, wouldnt the map be empty, too? so we'd end up loading them until we add some var? Should we have a null-reference to the map as long as it wasnt loaded? |
||
return environmentVariablesMap; | ||
} | ||
|
||
private Map<String, IEnvironmentVariable> loadEnvironmentVariables() throws OpenShiftException { | ||
|
@@ -624,13 +627,14 @@ private Map<String, IEnvironmentVariable> loadEnvironmentVariables() throws Open | |
return new LinkedHashMap<String, IEnvironmentVariable>(); | ||
} | ||
|
||
Map<String, IEnvironmentVariable> environmentVariablesByName = new LinkedHashMap<String, IEnvironmentVariable>(); | ||
for (EnvironmentVariableResourceDTO environmentVariableResourceDTO : environmentVariableDTOs) { | ||
final IEnvironmentVariable environmentVariable = | ||
new EnvironmentVariableResource(environmentVariableResourceDTO, this); | ||
environmentVariablesByName.put(environmentVariable.getName(), environmentVariable); | ||
|
||
environmentVariablesMap.put(environmentVariable.getName(),environmentVariable); | ||
|
||
} | ||
return environmentVariablesByName; | ||
return environmentVariablesMap; | ||
} | ||
|
||
@Override | ||
|
@@ -643,42 +647,67 @@ public IEnvironmentVariable addEnvironmentVariable(String name, String value) th | |
} | ||
if (hasEnvironmentVariable(name)) { | ||
throw new OpenShiftException("Environment variable with name \"{0}\" already exists.", name); | ||
} | ||
|
||
} | ||
EnvironmentVariableResourceDTO environmentVariableResourceDTO = | ||
new AddEnvironmentVariableRequest().execute(name, value); | ||
new AddEnvironmentVariableRequest().execute(name, value); | ||
IEnvironmentVariable environmentVariable = new EnvironmentVariableResource(environmentVariableResourceDTO, this); | ||
updateEnvironmentVariables(); | ||
|
||
environmentVariablesMap.put(environmentVariable.getName(), environmentVariable); | ||
|
||
return environmentVariable; | ||
} | ||
|
||
@Override | ||
public Map<String, IEnvironmentVariable> addEnvironmentVariables(Map<String, String> environmentVariablesMap) | ||
public Map<String, IEnvironmentVariable> addEnvironmentVariables(Map<String, String> environmentVariables) | ||
throws OpenShiftException { | ||
|
||
List<EnvironmentVariableResourceDTO> environmentVariableResourceDTOs = new AddEnvironmentVariablesRequest() | ||
.execute(environmentVariablesMap); | ||
Map<String, IEnvironmentVariable> environmentVariables = new HashMap<String, IEnvironmentVariable>(); | ||
|
||
Map<String,String>variablesCandidateMap = new HashMap<String,String>(); | ||
for(String varCandidateName:environmentVariables.keySet()){ | ||
IEnvironmentVariable tempVar = environmentVariablesMap.get(varCandidateName); | ||
if(tempVar != null) | ||
{ if(tempVar.getValue() == environmentVariables.get(varCandidateName)) | ||
variablesCandidateMap.put(varCandidateName,environmentVariables.get(varCandidateName)); | ||
} | ||
else | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we're filing single add-requests for each addition. It turns out that we could mutate the existing map and send it as a whole (or even better: only the changed parts in a single request). But nevermind, we can do this in a later change. |
||
variablesCandidateMap.put(varCandidateName, environmentVariables.get(varCandidateName)); | ||
} | ||
List<EnvironmentVariableResourceDTO> environmentVariableResourceDTOs = new AddEnvironmentVariablesRequest() | ||
.execute(variablesCandidateMap); | ||
|
||
for (EnvironmentVariableResourceDTO dto : environmentVariableResourceDTOs) { | ||
IEnvironmentVariable environmentVariable = new EnvironmentVariableResource(dto, this); | ||
environmentVariables.put(environmentVariable.getName(), environmentVariable); | ||
environmentVariablesMap.put(environmentVariable.getName(), environmentVariable); | ||
} | ||
|
||
updateEnvironmentVariables(); | ||
return environmentVariables; | ||
|
||
return environmentVariablesMap; | ||
} | ||
|
||
/* | ||
* (non-Javadoc) | ||
* @see com.openshift.client.IApplication#removeEnvironmentVariable(java.lang.String) | ||
*/ | ||
@Override | ||
public void removeEnvironmentVariable(String name) { | ||
IEnvironmentVariable environmentVariable = getEnvironmentVariable(name); | ||
if (environmentVariable == null) { | ||
return; | ||
} | ||
|
||
environmentVariable.destroy(); | ||
updateEnvironmentVariables(); | ||
public void removeEnvironmentVariable(String targetName) { | ||
removeEnvironmentVariable(getEnvironmentVariable(targetName)); | ||
} | ||
|
||
/* (non-Javadoc) | ||
* @see com.openshift.client.IApplication#removeEnvironmentVariable(com.openshift.client.IEnvironmentVariable) | ||
*/ | ||
@Override | ||
public void removeEnvironmentVariable(IEnvironmentVariable environmentVariable){ | ||
if(getEnvironmentVariable(environmentVariable.getName()) == null) | ||
throw new OpenShiftException("IEnvironmentVariable with supplied name does not exist."); | ||
environmentVariable.destroy(); | ||
environmentVariablesMap.remove(environmentVariable.getName()); | ||
|
||
} | ||
|
||
|
||
/* | ||
* (non-Javadoc) | ||
* @see com.openshift.client.IApplication#hasEnvironmentVariable(java.lang.String) | ||
*/ | ||
@Override | ||
public boolean hasEnvironmentVariable(String name) throws OpenShiftException { | ||
if (StringUtils.isEmpty(name)) { | ||
|
@@ -689,17 +718,20 @@ public boolean hasEnvironmentVariable(String name) throws OpenShiftException { | |
} | ||
|
||
protected void updateEnvironmentVariables() throws OpenShiftException { | ||
if (!canGetEnvironmentVariables()) { | ||
if (!canGetEnvironmentVariables()) | ||
return; | ||
else | ||
{ | ||
environmentVariablesMap.clear(); | ||
environmentVariablesMap = loadEnvironmentVariables(); | ||
} | ||
if (environmentVariableByName == null) { | ||
environmentVariableByName = loadEnvironmentVariables(); | ||
} else { | ||
environmentVariableByName.clear(); | ||
environmentVariableByName.putAll(loadEnvironmentVariables()); | ||
} | ||
|
||
} | ||
|
||
/* | ||
* (non-Javadoc) | ||
* @see com.openshift.client.IApplication#getEnvironmentVariable(java.lang.String) | ||
*/ | ||
@Override | ||
public IEnvironmentVariable getEnvironmentVariable(String name) { | ||
return getEnvironmentVariables().get(name); | ||
|
@@ -714,6 +746,10 @@ public boolean canGetEnvironmentVariables() { | |
} | ||
} | ||
|
||
/* | ||
* (non-Javadoc) | ||
* @see com.openshift.client.IApplication#canUpdateEnvironmentVariables() | ||
*/ | ||
@Override | ||
public boolean canUpdateEnvironmentVariables() { | ||
try { | ||
|
@@ -820,9 +856,11 @@ public List<IApplicationPortForwarding> startPortForwarding() throws OpenShiftSS | |
try { | ||
port.start(session); | ||
} catch (OpenShiftSSHOperationException oss) { | ||
// ignore for now | ||
// FIXME: should store this error on the forward to let user | ||
// know why it could not start/stop | ||
/* | ||
* ignore for now | ||
* FIXME: should store this error on the forward to let user | ||
* know why it could not start/stop | ||
*/ | ||
} | ||
} | ||
return ports; | ||
|
@@ -833,9 +871,10 @@ public List<IApplicationPortForwarding> stopPortForwarding() throws OpenShiftSSH | |
try { | ||
port.stop(session); | ||
} catch (OpenShiftSSHOperationException oss) { | ||
// ignore for now | ||
// should store this error on the forward to let user know why | ||
// it could not start/stop | ||
/* ignore for now | ||
* should store this error on the forward to let user know why | ||
* it could not start/stop | ||
*/ | ||
} | ||
} | ||
// make sure port forwarding is stopped by closing session... | ||
|
@@ -1094,5 +1133,4 @@ protected List<EnvironmentVariableResourceDTO> execute(Map<String, String> envir | |
} | ||
} | ||
|
||
|
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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 .