You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/04/26 13:56:47 UTC
[GitHub] [maven-surefire] akomakom opened a new pull request #287: [DO NOT MERGE] Exclude explicitly configured env vars
akomakom opened a new pull request #287:
URL: https://github.com/apache/maven-surefire/pull/287
tests forthcoming
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] akomakom commented on a change in pull request #287: [DO NOT MERGE] SUREFIRE-1782 Exclude explicitly configured env vars
Posted by GitBox <gi...@apache.org>.
akomakom commented on a change in pull request #287:
URL: https://github.com/apache/maven-surefire/pull/287#discussion_r415321091
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/lazytestprovider/OutputStreamFlushableCommandline.java
##########
@@ -53,19 +56,27 @@ public OutputStreamFlushableCommandline()
public OutputStreamFlushableCommandline( String[] excludedEnvironmentVariables )
{
this.excludedEnvironmentVariables = new ConcurrentLinkedDeque<>();
+ this.addedEnvironmentVariables = new HashSet<>( );
Review comment:
I'm still tuning my intellij settings for surefire checkstyle...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #287: [DO NOT MERGE] SUREFIRE-1782 Exclude explicitly configured env vars
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #287:
URL: https://github.com/apache/maven-surefire/pull/287#discussion_r415318730
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/lazytestprovider/OutputStreamFlushableCommandline.java
##########
@@ -53,19 +56,27 @@ public OutputStreamFlushableCommandline()
public OutputStreamFlushableCommandline( String[] excludedEnvironmentVariables )
{
this.excludedEnvironmentVariables = new ConcurrentLinkedDeque<>();
+ this.addedEnvironmentVariables = new HashSet<>( );
Review comment:
try to rewrite to `addedEnvironmentVariables = new HashSet<>();`
without `this` and space in constructor.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #287: [DO NOT MERGE] SUREFIRE-1782 Exclude explicitly configured env vars
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #287:
URL: https://github.com/apache/maven-surefire/pull/287#discussion_r415319324
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/lazytestprovider/OutputStreamFlushableCommandline.java
##########
@@ -53,19 +56,27 @@ public OutputStreamFlushableCommandline()
public OutputStreamFlushableCommandline( String[] excludedEnvironmentVariables )
{
this.excludedEnvironmentVariables = new ConcurrentLinkedDeque<>();
+ this.addedEnvironmentVariables = new HashSet<>( );
Collections.addAll( this.excludedEnvironmentVariables, excludedEnvironmentVariables );
}
+ @Override
+ public void addEnvironment( String name, String value )
+ {
+ super.addEnvironment( name, value );
+ addedEnvironmentVariables.add( name );
+ }
+
@Override
public final void addSystemEnvironment()
{
Properties systemEnvVars = CommandLineUtils.getSystemEnvVars();
for ( String key : systemEnvVars.stringPropertyNames() )
{
- if ( !excludedEnvironmentVariables.contains( key ) )
+ if ( !addedEnvironmentVariables.contains( key ) && !excludedEnvironmentVariables.contains( key ) )
{
- addEnvironment( key, systemEnvVars.getProperty( key ) );
+ super.addEnvironment( key, systemEnvVars.getProperty( key ) );
Review comment:
Is it so necessary to use `super`?
If the environment is set once, it cannot be changed otherway. It is the original behavior in the library.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #287: [DO NOT MERGE] SUREFIRE-1782 Exclude explicitly configured env vars
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #287:
URL: https://github.com/apache/maven-surefire/pull/287#issuecomment-620021601
ok, thx
let me know when it is ready to merge and we will change the title.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] ngeor commented on pull request #287: SUREFIRE-1782 Exclude explicitly configured env vars
Posted by GitBox <gi...@apache.org>.
ngeor commented on pull request #287:
URL: https://github.com/apache/maven-surefire/pull/287#issuecomment-705620915
Hello, not sure if this is related, but I have an issue with environment variables in failsafe that starts with M5 (works fine on M4).
In pom, I declare an environment variable:
```
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-failsafe-plugin</artifactId>
<executions>
<execution>
<goals>
<goal>integration-test</goal>
<goal>verify</goal>
</goals>
</execution>
</executions>
<configuration>
<environmentVariables>
<DB_PATH>src/test/resources/db-local.json</DB_PATH>
</environmentVariables>
</configuration>
</plugin>
```
In the CI, I override this environment variable. After upgrading to M5, the override does not work anymore.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #287: [DO NOT MERGE] Exclude explicitly configured env vars
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #287:
URL: https://github.com/apache/maven-surefire/pull/287#discussion_r415317010
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/lazytestprovider/OutputStreamFlushableCommandline.java
##########
@@ -53,19 +56,27 @@ public OutputStreamFlushableCommandline()
public OutputStreamFlushableCommandline( String[] excludedEnvironmentVariables )
{
this.excludedEnvironmentVariables = new ConcurrentLinkedDeque<>();
+ this.addedEnvironmentVariables = new HashSet<>( );
Collections.addAll( this.excludedEnvironmentVariables, excludedEnvironmentVariables );
}
+ @Override
+ public void addEnvironment( String name, String value )
+ {
+ super.addEnvironment( name, value );
+ addedEnvironmentVariables.add( name );
+ }
+
@Override
public final void addSystemEnvironment()
{
Properties systemEnvVars = CommandLineUtils.getSystemEnvVars();
for ( String key : systemEnvVars.stringPropertyNames() )
{
- if ( !excludedEnvironmentVariables.contains( key ) )
+ if ( !addedEnvironmentVariables.contains( key ) && !excludedEnvironmentVariables.contains( key ) )
Review comment:
yes, this is what I mean. LGTM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] akomakom commented on pull request #287: SUREFIRE-1782 Exclude explicitly configured env vars
Posted by GitBox <gi...@apache.org>.
akomakom commented on pull request #287:
URL: https://github.com/apache/maven-surefire/pull/287#issuecomment-620087672
@Tibor17 I think this is ready to merge, I was going to look into tests but haven't had much time. I tested this locally (manually), and also #288 works (locally) as expected on top of this change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] ngeor commented on pull request #287: SUREFIRE-1782 Exclude explicitly configured env vars
Posted by GitBox <gi...@apache.org>.
ngeor commented on pull request #287:
URL: https://github.com/apache/maven-surefire/pull/287#issuecomment-705620915
Hello, not sure if this is related, but I have an issue with environment variables in failsafe that starts with M5 (works fine on M4).
In pom, I declare an environment variable:
```
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-failsafe-plugin</artifactId>
<executions>
<execution>
<goals>
<goal>integration-test</goal>
<goal>verify</goal>
</goals>
</execution>
</executions>
<configuration>
<environmentVariables>
<DB_PATH>src/test/resources/db-local.json</DB_PATH>
</environmentVariables>
</configuration>
</plugin>
```
In the CI, I override this environment variable. After upgrading to M5, the override does not work anymore.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #287: SUREFIRE-1782 Exclude explicitly configured env vars
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #287:
URL: https://github.com/apache/maven-surefire/pull/287#issuecomment-620187228
@akomakom
Thx for contributing again 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org