Allow clients to pass in environment for devappserver runs #381
Conversation
…devappserver1 runs
…node in attribute map construction
…to package-private too
|
Aha, the penny has dropped a little further. This GcloudArgs library is
inconsistent in who is reponsible for including the '--flagName', with the
map support leaving it to the client to prepend.
How about just 'flaggedKeyValues", and relying on the javadoc to resolve
confusion?
I'm flumoxed by the keyValues implementation. It seems to always return a
single entry list.. Why doesn't it just return string? The lack of "" in
the javadoc return comment compounds the confusion.
…On Tue, Apr 25, 2017 at 3:24 PM, Etan Shaul ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/main/java/com/google/cloud/tools/appengine/cloudsdk/internal/args/
DevAppServerArgs.java
<#381 (comment)>
:
> @@ -71,4 +72,12 @@
}
return Collections.emptyList();
}
+
+ /**
+ * @return ***@***.*** [--name, key1=val1, --name, key2=val2, ...]} or ***@***.*** []} if
+ * keyValues=empty/null.
+ */
+ public static List<String> get(String name, Map<String, String> keyValues) {
+ return Args.namedKeyValues(name, keyValues);
flagPerEntry isn't bad; but what would you suggest for the sister method
that returns the entry without the flag? namedKeyValues is a bit
confusing I agree, but when paired with keyValues it because a little
clearer.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#381 (comment)>,
or mute the thread
</~https://github.com/notifications/unsubscribe-auth/AHf5Hb6LxDw2XiGKLo-hHCpjwvtNJM-kks5rzkhfgaJpZM4NEp7K>
.
|
| @@ -172,7 +177,7 @@ public void setPythonStartupArgs(String pythonStartupArgs) { | |||
| } | |||
|
|
|||
| public void setJvmFlags(List<String> jvmFlags) { | |||
| this.jvmFlags = jvmFlags; | |||
| this.jvmFlags = jvmFlags != null ? ImmutableList.copyOf(jvmFlags) : null; | |||
There was a problem hiding this comment.
I wonder if we should do safer copying of lists everywhere?
It's a good idea, but is the expectation that a user would change their environment after setting this?
There was a problem hiding this comment.
For the first question - yeah probably. I only touched this field unrelated to this PR because it's in same class. We can deal with the rest of the cases in a different PR.
No I don't think it's expected that they change it. It's just to protect against them unknowingly (or knowingly even but for other reasons) changing it and getting unexpected side effects. Maybe I misunderstood your point.
| @@ -61,7 +61,7 @@ public void setTrafficTest() throws ProcessRunnerException { | |||
| appEngineService.setTraffic(configuration); | |||
|
|
|||
| List<String> args = | |||
| Arrays.asList("services", "set-traffic", "myService", "--splits", "v1=0.3,v2=0.7", | |||
| Arrays.asList("services", "set-traffic", "myService", "--splits", "v1=0.3", "v2=0.7", | |||
There was a problem hiding this comment.
I dont understand how this would work.
There was a problem hiding this comment.
does splits accept multiple parameters after itself?
There was a problem hiding this comment.
Yeah I think so. Or did at least when I first implemented this. I'll check the current state.
There was a problem hiding this comment.
ohh. good eye! I think my change may have subtly broke this. It accepts the following format:
--splits v2=.5,v1=.5
With my update it will be space separated. I'll revisit this.
|
@loosebazooka fixed: cb0ecc0 @patflynn turns out you were right to be suspicious that the updates to the keyValue method were client-breaking. Thanks for catching. |
|
@patflynn & @loosebazooka, I know you approved already but do you mind taking a quick last look given the last change I had to make (see the last couple of comments). |
| */ | ||
| public static List<String> keyValues(Map<?, ?> keyValueMapping) { | ||
| static List<String> keyValueString(Map<?, ?> keyValueMapping) { |
There was a problem hiding this comment.
can you safely make this non public?
There was a problem hiding this comment.
what do you mean? There were no non-package private usages of it.
There was a problem hiding this comment.
also the class itself is package private
|
lgtm 🏄♂️ |
# Conflicts: # src/test/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdkAppEngineDevServer2Test.java
fixes #379
Note the this depends on #378 so the target branch is the branch of that PR. I'll update the target branch once #378 is merged in.
I added the environment just to the config of the devappserver. I didn't want to add it to the CloudSdk configuration (as its possible that the object is reused for multiple commands and we don't necessarily want to share these env variables across commands). Also, this is closing a gap we have with the devappserver so I wanted to limit it to that.