Extract environment settings from appengine-web.xml and set them for devappserver1 runs #378

etanshaul · 2017-04-19T18:23:05Z

What do you think of this approach?

If ok, I'll go ahead and add unit tests.

I tested this with devappserver1 and IntelliJ.

…devappserver1 runs

etanshaul · 2017-04-19T18:24:09Z

src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdkAppEngineDevServer1.java

+  Map<String, String> getAllAppEngineEnvironment(List<File> services) {
+    Map<String, String> allAppEngineEnvironment = Maps.newHashMap();
+    for (File serviceDirectory : services) {
+      Path appengineWebXml = serviceDirectory.toPath().resolve("WEB-INF/appengine-web.xml");


I know theres some duplication here with isJava8; looking at refactoring a little.

patflynn · 2017-04-19T18:25:46Z

The fact that the environment variables can clash between services looks like a problem for the devappserver1 multi-module model.

…

On Wed, Apr 19, 2017 at 2:24 PM, Etan Shaul ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/java/com/google/cloud/tools/appengine/cloudsdk/ CloudSdkAppEngineDevServer1.java <#378 (comment)> : > @@ -194,4 +199,22 @@ boolean isJava8(List<File> services) { } return java8Detected; } + + @VisibleForTesting + Map<String, String> getAllAppEngineEnvironment(List<File> services) { + Map<String, String> allAppEngineEnvironment = Maps.newHashMap(); + for (File serviceDirectory : services) { + Path appengineWebXml = serviceDirectory.toPath().resolve("WEB-INF/appengine-web.xml"); I know theres some duplication here with isJava8; looking at refactoring a little. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#378 (review)>, or mute the thread </~https://github.com/notifications/unsubscribe-auth/AHf5HRecWder1GtvSMGUclEMky6QU8n7ks5rxlFKgaJpZM4NCDy9> .

etanshaul · 2017-04-19T18:26:49Z

That's true. However isn't that a problem for devappserver2 as well? I wonder how it handles it.

etanshaul · 2017-04-19T18:28:22Z

src/main/java/com/google/cloud/tools/appengine/AppEngineDescriptor.java

+    return null;
+  }
+
+  private static Map<String, String> getAttributeMap(Node node, String key, String value) {


I realized I need to update this to also search for a target child node by name. E.g.:

<env-variables> <env-var name="ENDPOINTS_SERVICE_NAME" value="some_value" /> </env-variables>

The logic should probably search for the tag 'env-var'.

patflynn · 2017-04-19T18:30:21Z

devappserver2 uses a different jvm for each service. In any case this seems better than what we have now. Even if there's potential for a clash.

…

On Wed, Apr 19, 2017 at 2:28 PM, Etan Shaul ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/java/com/google/cloud/tools/appengine/AppEngineDescriptor.java <#378 (comment)> : > @@ -99,16 +105,55 @@ public boolean isJava8() { && getRuntime().startsWith("java8"); } - private static String getTopLevelValue(Document doc, String parentTagName, String childTagName) { + public Map<String, String> getEnvironment() { + Node environmentParentNode = getTargetNode(document, "appengine-web-app", "env-variables"); + return getAttributeMap(environmentParentNode, "name", "value"); + } + + private static String getText(Node node) { + if (node != null) { + return node.getTextContent(); + } + + return null; + } + + private static Map<String, String> getAttributeMap(Node node, String key, String value) { I realized I need to update this to also search for a target child node by name. E.g.: <env-variables> <env-var name="ENDPOINTS_SERVICE_NAME" value="some_value" /> </env-variables> The logic should probably search for the tag 'env-var'. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#378 (review)>, or mute the thread </~https://github.com/notifications/unsubscribe-auth/AHf5HTo-UTc-Laos-rrL6H1tMHaj0bhdks5rxlJHgaJpZM4NCDy9> .

etanshaul · 2017-04-19T18:31:33Z

oh, I see. yeah the effect of the clash here is simply to use the value from the last service in the list.

patflynn · 2017-04-19T18:35:28Z

that's probably worth warning on.

…

On Apr 19, 2017 2:31 PM, "Etan Shaul" ***@***.***> wrote: oh, I see. yeah the effect of the clash here is simply to use the value from the last service in the list. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#378 (comment)>, or mute the thread </~https://github.com/notifications/unsubscribe-auth/AHf5HQU7rTU1ok1aOPEosJXu5h44dNjaks5rxlMIgaJpZM4NCDy9> .

…node in attribute map construction

codecov-io · 2017-04-19T19:26:17Z

Codecov Report

Merging #378 into master will increase coverage by 0.47%.
The diff coverage is 79.41%.

@@            Coverage Diff            @@
##           master    #378      +/-   ##
=========================================
+ Coverage   63.82%   64.3%   +0.47%     
=========================================
  Files          66      66              
  Lines        1703    1751      +48     
  Branches      252     264      +12     
=========================================
+ Hits         1087    1126      +39     
- Misses        508     513       +5     
- Partials      108     112       +4

Impacted Files	Coverage Δ
...oogle/cloud/tools/appengine/cloudsdk/CloudSdk.java	`40.76% <0%> (ø)`	⬆️
...gle/cloud/tools/appengine/AppEngineDescriptor.java	`76.47% <76.92%> (-5.68%)`	⬇️
...ppengine/cloudsdk/CloudSdkAppEngineDevServer1.java	`85.71% <92.3%> (+1.67%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 814aca9...55a5d75. Read the comment docs.

etanshaul · 2017-04-21T19:45:18Z

@elharo since you previously commented, are you good with this now?

elharo · 2017-04-21T20:05:02Z

src/main/java/com/google/cloud/tools/appengine/AppEngineDescriptor.java

+      Map<String, String> nameValueAttributeMap = Maps.newHashMap();
+
+      if (parent.hasChildNodes()) {
+        for (int i = 0; i < parent.getChildNodes().getLength(); ++i) {


i++ is more common

yea, not sure why its like that, Ill update

Kinda disappointing that we can't use the shortened for loop syntax here 😞

elharo · 2017-04-21T20:06:13Z

src/main/java/com/google/cloud/tools/appengine/AppEngineDescriptor.java

+  }
+
+  /**
+   * Returns a map formed from the attributes of the nodes contained within the parent node.


what if two child elements have attributes with the same name?

then the last one found will be added to the map (according to the contract of Map#put)

briandealwis · 2017-04-21T20:33:18Z

src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdk.java

@@ -292,7 +293,6 @@ public void runDevAppServer1Command(List<String> jvmArgs, List<String> args)

    logCommand(command);

-    Map<String, String> environment = Maps.newHashMap();
    environment.put("JAVA_HOME", javaHomePath.toAbsolutePath().toString());


Shouldn't we be copying the provided environment and avoid side-effects?

etanshaul · 2017-04-24T14:45:27Z

@elharo @briandealwis please take a look to see if I've addressed all your comments.

briandealwis · 2017-04-24T15:50:21Z

src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdkAppEngineDevServer1.java

+                                                        Map<String, String> existingEnvironment,
+                                                        String service) {
+    for (String key : newEnvironment.keySet()) {
+      if (existingEnvironment.containsKey(key)) {


Downside to this detection approach is that it doesn't provide the source of the original definitions. But this can be left as a todo for later.

yea, I was aware of this when implementing it. I didn't think it was that essential to justify the extra logic to deliver this (knowing one of the offending files to me was adequate). But yea, can be a todo.

briandealwis · 2017-04-24T15:52:49Z

src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdkAppEngineDevServer1.java

+        Map<String, String> appEngineEnvironment = AppEngineDescriptor.parse(is).getEnvironment();
+        if (appEngineEnvironment != null) {
+          checkAndWarnDuplicateEnvironmentVariables(
+              appEngineEnvironment, allAppEngineEnvironment, serviceDirectory.getName());


Since you are parsing the appengine-web.xml, why not report the real service name rather than the directory? The directory name may not correspond to the service name.

@briandealwis updated

etanshaul · 2017-04-24T19:01:12Z

@briandealwis / @elharo do I have the ok? I'd love to merge this in today if you have no further comments.

patflynn · 2017-04-24T19:12:40Z

@etanshaul
@elharo
@briandealwis

What do you guys think of a log.info message tell the user we're setting these environment variables?

briandealwis · 2017-04-24T19:38:39Z

LGTM @etanshaul

+1 to logging the environment variables: that would help the developer figure out where values are coming from.

etanshaul · 2017-04-24T20:26:25Z

@patflynn, added a logging statement ca8678e

patflynn · 2017-04-24T20:54:14Z

src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdk.java

-    processRunner.setEnvironment(environment);
+    Map<String, String> devServerEnvironment = Maps.newHashMap(environment);
+    if (!devServerEnvironment.isEmpty()) {
+      logger.info("Setting user supplied environment variables: "


nit: I think you need to let them know this is coming from appengine-web.xml :)

patflynn · 2017-04-24T20:59:45Z

src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdk.java

-    processRunner.setEnvironment(environment);
+    Map<String, String> devServerEnvironment = Maps.newHashMap(environment);
+    if (!devServerEnvironment.isEmpty()) {
+      logger.info("Setting appengine-web.xml configured environment variables: "


OK now I am being a total PITA but I worry that some other entrypoint might pass environment that's not based off of appengine-web.xml. Perhaps this logging statement belongs closer to CloudSdkAppEngineDevServer1:120?

Feel free to throw darts at an effigy of me.

no, you're right. I was planning on improving this in the other PR (that allows client supplied env vars) since we'll have to differentiate between them. But yea, I think maybe moving them to the other class is the right approach.

…e place

elharo · 2017-04-24T21:08:03Z

src/main/java/com/google/cloud/tools/appengine/AppEngineDescriptor.java

+   * Returns the first node found matching the given name contained within the parent node.
+   */
+  private static Node getNode(Document doc, String parentNodeName, String targetNodeName) {
+    NodeList parentElements = doc.getElementsByTagNameNS(APP_ENGINE_NAMESPACE, parentNodeName);


I'm not sure why you're using the parentNodeName here. You could jump straight to the relevant nodes.

I figured this helps to ensure the xml for the env vars is properly formed: e.g. env-var elements contained within a parent env-variables. If I skipped straight to env-vars then wouldn't it match against invalid parent elements?

Also, this method is used by all the extractors (service, version etc. Not just for environment). Its pretty much a direct refactor of what was there before (getTopLevelValue as it was previously called).

elharo · 2017-04-24T21:09:32Z

src/main/java/com/google/cloud/tools/appengine/AppEngineDescriptor.java

+      if (parent.hasChildNodes()) {
+        for (int i = 0; i < parent.getChildNodes().getLength(); i++) {
+          Node child = parent.getChildNodes().item(i);
+          if (child.getNodeName().equals(targetNodeName)) {


Theoretically you should be checking namesapces here too. Probalby doesn't matter much in practice. However there are a lot of ways this navigtion could go wrong is the appen gine-web.xml file is not as required.

could you be more specific regarding your last sentence? e.g. what could go wrong and do you think I should be handling it here?

elharo · 2017-04-24T21:11:27Z

src/test/resources/projects/Standard8Project1EnvironmentVariables/WEB-INF/appengine-web.xml

@@ -0,0 +1,31 @@
+<?xml version="1.0" encoding="utf-8"?>


You can make these test data file smaller if all they're used for is to test the environment variables.

etanshaul · 2017-04-25T18:03:25Z

discussed with @elharo offline and we are good to go with this. will merge

…devappserver1 runs (#378)

* Remove v2-alpha devappserver support * Admin Override: MacOS test could be broken (because of test machine jdk config)

Extract environment settings from appengine-web.xml and set them for …

c4c341b

…devappserver1 runs

googlebot added the cla: yes label Apr 19, 2017

etanshaul requested a review from loosebazooka April 19, 2017 18:23

etanshaul assigned akerekes and patflynn and unassigned akerekes and patflynn Apr 19, 2017

etanshaul requested review from akerekes and patflynn April 19, 2017 18:23

etanshaul commented Apr 19, 2017

View reviewed changes

move exception catching to where relevant code; add check for target …

5c356bc

…node in attribute map construction

etanshaul requested a review from elharo April 19, 2017 18:46

etanshaul added 9 commits April 19, 2017 14:51

doc

dfb479d

doc

e51cb20

typo

ed4c283

formatting

4c0f11a

var naming

60885af

variable naming; fix test

78dbb73

fix formatting

d392fe2

naming & formatting

01da7a2

quiet checkstyle

841a027

etanshaul added 2 commits April 19, 2017 15:31

more javadoc

abee6f3

add tests

03dc9af

etanshaul mentioned this pull request Apr 21, 2017

Allow clients to pass in environment for devappserver runs #381

Merged

patflynn approved these changes Apr 21, 2017

View reviewed changes

elharo reviewed Apr 21, 2017

View reviewed changes

briandealwis reviewed Apr 21, 2017

View reviewed changes

etanshaul added 3 commits April 24, 2017 10:36

merge and delete duplicated unit test

3502293

make copy of environment instead of mutating it

8ba4c67

pre to post increment index

4a9e873

briandealwis reviewed Apr 24, 2017

View reviewed changes

warn on dup env vars using the real service name

5ce1b26

add log.info for setting env vars

ca8678e

patflynn reviewed Apr 24, 2017

View reviewed changes

update log statement to say it comes from appengine-web.xml

374950e

patflynn reviewed Apr 24, 2017

View reviewed changes

move appengine-web.xml env var logging statement to a more appropriat…

95c4684

…e place

patflynn approved these changes Apr 24, 2017

View reviewed changes

elharo reviewed Apr 24, 2017

View reviewed changes

etanshaul added 2 commits April 24, 2017 17:21

shrink test appengine-web.xml files that are only testing env vars

b1c3edb

javadoc

55a5d75

etanshaul merged commit 3ebe475 into master Apr 25, 2017

loosebazooka mentioned this pull request May 5, 2017

Upgrade to plugins-core 0.3.2 GoogleCloudPlatform/app-gradle-plugin#127

Closed

chanseokoh deleted the set-environment branch May 5, 2017 17:46

JoeWang1127 pushed a commit that referenced this pull request Nov 29, 2023

Extract environment settings from appengine-web.xml and set them for …

ccf3285

…devappserver1 runs (#378)

JoeWang1127 pushed a commit that referenced this pull request Nov 29, 2023

Remove v2-alpha devappserver support (#378)

35ecc47

* Remove v2-alpha devappserver support * Admin Override: MacOS test could be broken (because of test machine jdk config)

Extract environment settings from appengine-web.xml and set them for devappserver1 runs #378

Extract environment settings from appengine-web.xml and set them for devappserver1 runs #378

Conversation

etanshaul commented Apr 19, 2017

Choose a reason for hiding this comment

patflynn commented Apr 19, 2017 via email

etanshaul commented Apr 19, 2017

Choose a reason for hiding this comment

patflynn commented Apr 19, 2017 via email

etanshaul commented Apr 19, 2017

patflynn commented Apr 19, 2017 via email

codecov-io commented Apr 19, 2017 • edited Loading

Codecov Report

etanshaul commented Apr 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etanshaul commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etanshaul commented Apr 24, 2017

patflynn commented Apr 24, 2017

briandealwis commented Apr 24, 2017 • edited Loading

etanshaul commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etanshaul commented Apr 25, 2017

codecov-io commented Apr 19, 2017 •

edited

Loading

briandealwis commented Apr 24, 2017 •

edited

Loading