Skip to content

Commit

Permalink
gocd#871 - print env. variables at start of job
Browse files Browse the repository at this point in the history
  • Loading branch information
@srinivasupadhya
srinivasupadhya authored and sriniup committed Feb 4, 2015
1 parent 32e0b5f commit 9d03d3e
Show file tree
Hide file tree
Showing 5 changed files with 182 additions and 0 deletions.
1 change: 1 addition & 0 deletions common/src/com/thoughtworks/go/remote/work/BuildWork.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ private EnvironmentVariableContext setupEnvrionmentContext(EnvironmentVariableCo
private JobResult buildJob(EnvironmentVariableContext environmentVariableContext) {
goPublisher.reportCurrentStatus(JobState.Building);
goPublisher.reportAction("Start to build");
goPublisher.reportEnvironmentVariables(environmentVariableContext);
return execute(environmentVariableContext);
}

Expand Down
19 changes: 19 additions & 0 deletions common/src/com/thoughtworks/go/work/DefaultGoPublisher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
package com.thoughtworks.go.work;

import java.io.File;
import java.util.HashSet;
import java.util.Set;

import com.thoughtworks.go.domain.builder.FetchArtifactBuilder;
import com.thoughtworks.go.domain.JobIdentifier;
Expand All @@ -31,6 +33,7 @@
import com.thoughtworks.go.util.GoConstants;
import com.thoughtworks.go.util.SystemUtil;
import com.thoughtworks.go.util.TimeProvider;
import com.thoughtworks.go.util.command.EnvironmentVariableContext;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

Expand Down Expand Up @@ -142,4 +145,20 @@ public void reportErrorMessage(String message, Exception e) {
LOG.error(message, e);
consumeLine(message);
}

public void reportEnvironmentVariables(EnvironmentVariableContext environmentVariableContext) {
Set<String> environmentVariableNames = new HashSet<String>();
String line;

for (EnvironmentVariableContext.EnvironmentVariable environmentVariable : environmentVariableContext.getEnvironmentVariables()) {
if (environmentVariableNames.contains(environmentVariable.name())) {

This comment has been minimized.

Sign in to view

Copy link
@zabil

zabil Feb 9, 2015

check comment about getEnvironmentVariables
line = format("[%s] overriding environment variable '%s' with value '%s'", GoConstants.PRODUCT_NAME, environmentVariable.name(), environmentVariable.valueForDisplay());
} else {
line = format("[%s] setting environment variable '%s' to value '%s'", GoConstants.PRODUCT_NAME, environmentVariable.name(), environmentVariable.valueForDisplay());
}
consumeLine(line);

environmentVariableNames.add(environmentVariable.name());
}
}
}
142 changes: 142 additions & 0 deletions common/test/com/thoughtworks/go/work/DefaultGoPublisherTest.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
/*************************GO-LICENSE-START*********************************
* Copyright 2014 ThoughtWorks, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*************************GO-LICENSE-END***********************************/

package com.thoughtworks.go.work;

import com.thoughtworks.go.domain.JobIdentifier;
import com.thoughtworks.go.publishers.GoArtifactsManipulator;
import com.thoughtworks.go.remote.AgentIdentifier;
import com.thoughtworks.go.remote.BuildRepositoryRemote;
import com.thoughtworks.go.remote.work.ConsoleAppender;
import com.thoughtworks.go.remote.work.ConsoleOutputTransmitter;
import com.thoughtworks.go.server.service.AgentRuntimeInfo;
import com.thoughtworks.go.util.SystemEnvironment;
import com.thoughtworks.go.util.command.EnvironmentVariableContext;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.mockito.Mock;

import java.io.IOException;

import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.mockito.MockitoAnnotations.initMocks;

public class DefaultGoPublisherTest {
private static final String PROPERTY_NAME = "PROPERTY_NAME";
private static final String PROPERTY_VALUE = "property value";
private static final String NEW_VALUE = "new value";

@Mock
ConsoleAppender consoleAppender;
@Mock
AgentIdentifier agentIdentifier;
@Mock
JobIdentifier jobIdentifier;
@Mock
GoArtifactsManipulator goArtifactsManipulator;
@Mock
BuildRepositoryRemote buildRepositoryRemote;
@Mock
AgentRuntimeInfo agentRuntimeInfo;

ConsoleOutputTransmitter consoleOutputTransmitter;
DefaultGoPublisher defaultGoPublisher;

@Before
public void setUp() {
initMocks(this);

new SystemEnvironment().setProperty(SystemEnvironment.INTERVAL, "60"); // so the thread does not wake up

consoleOutputTransmitter = new ConsoleOutputTransmitter(consoleAppender);

when(agentRuntimeInfo.getIdentifier()).thenReturn(agentIdentifier);
when(goArtifactsManipulator.createConsoleOutputTransmitter(jobIdentifier, agentIdentifier)).thenReturn(consoleOutputTransmitter);

defaultGoPublisher = new DefaultGoPublisher(goArtifactsManipulator, jobIdentifier, buildRepositoryRemote, agentRuntimeInfo);
}

@After
public void tearDown() {
consoleOutputTransmitter.stop();
}

@Test
public void shouldReportWhenAVariableIsSet() throws Exception {
EnvironmentVariableContext context = new EnvironmentVariableContext();
context.setProperty(PROPERTY_NAME, PROPERTY_VALUE, false);

defaultGoPublisher.reportEnvironmentVariables(context);
consoleOutputTransmitter.flushToServer();

String line1 = "[go] setting environment variable 'PROPERTY_NAME' to value 'property value'\n";
verifyConsoleOutput(line1);
}

@Test
public void shouldReportSecureVariableAsMaskedValue() throws Exception {
EnvironmentVariableContext context = new EnvironmentVariableContext();
context.setProperty(PROPERTY_NAME, PROPERTY_VALUE, true);

defaultGoPublisher.reportEnvironmentVariables(context);
consoleOutputTransmitter.flushToServer();

String line1 = String.format("[go] setting environment variable 'PROPERTY_NAME' to value '%s'\n", EnvironmentVariableContext.EnvironmentVariable.MASK_VALUE);
verifyConsoleOutput(line1);
}

@Test
public void shouldReportWhenAVariableIsOverridden() throws Exception {
EnvironmentVariableContext context = new EnvironmentVariableContext();
context.setProperty(PROPERTY_NAME, PROPERTY_VALUE, false);
context.setProperty(PROPERTY_NAME, NEW_VALUE, false);

defaultGoPublisher.reportEnvironmentVariables(context);
consoleOutputTransmitter.flushToServer();

String line1 = "[go] setting environment variable 'PROPERTY_NAME' to value 'property value'\n";
String line2 = "[go] overriding environment variable 'PROPERTY_NAME' with value 'new value'\n";
verifyConsoleOutput(line1, line2);
}

@Test
public void shouldMaskOverRiddenSecureVariable() throws Exception {
EnvironmentVariableContext context = new EnvironmentVariableContext();
context.setProperty(PROPERTY_NAME, PROPERTY_VALUE, true);
context.setProperty(PROPERTY_NAME, NEW_VALUE, true);

defaultGoPublisher.reportEnvironmentVariables(context);
consoleOutputTransmitter.flushToServer();

String line1 = String.format("[go] setting environment variable 'PROPERTY_NAME' to value '%s'\n", EnvironmentVariableContext.EnvironmentVariable.MASK_VALUE);
String line2 = String.format("[go] overriding environment variable 'PROPERTY_NAME' with value '%s'\n", EnvironmentVariableContext.EnvironmentVariable.MASK_VALUE);
verifyConsoleOutput(line1, line2);
}

private void verifyConsoleOutput(String... lines) throws IOException {
verify(consoleAppender).append(join(lines));
}

private String join(String... lines) {
StringBuilder result = new StringBuilder();
for (String line : lines) {
result.append(line);
}
return result.toString();
}
}
4 changes: 4 additions & 0 deletions config/config-api/src/com/thoughtworks/go/util/command/EnvironmentVariableContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,10 @@ public String getProperty(String key) {
return null;
}

public List<EnvironmentVariable> getEnvironmentVariables() {
return properties;

This comment has been minimized.

Sign in to view

Copy link
@zabil

zabil Feb 9, 2015

I'd be cautious about exposing this list.
Is there a reason for not wrapping it in EnvironmentVariables instead?

EnvironmentVariables implements Iterable<EnvironmentVariable>
List<EnvironmentVariable> list;
constructor(<EnvironmentVariable>);
public iterator(){
    return list.iterator();
}

This way the logic at reportEnvironmentVariables can come here and return a string that needs to be displayed.
}

public List<EnvironmentVariable> getSecureEnvironmentVariables() {
List<EnvironmentVariable> environmentVariables = new ArrayList<EnvironmentVariable>();
for (EnvironmentVariable property : properties) {
Expand Down
16 changes: 16 additions & 0 deletions server/test/unit/com/thoughtworks/go/remote/work/BuildWorkTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,22 @@ public void shouldReportCurrentWorkingDirectory() throws Exception {
containsString("[" + SystemUtil.currentWorkingDirectory() + "]"));
}

@Test
public void shouldReportEnvironmentVariables() throws Exception {
buildWork = (BuildWork) getWork(WILL_PASS, PIPELINE_NAME);

buildWork.doWork(agentIdentifier, buildRepository, artifactManipulator, environmentVariableContext, AgentRuntimeInfo.fromAgent(agentIdentifier, "cookie", null), packageAsRepositoryExtension, scmExtension, taskExtension);

assertThat(artifactManipulator.consoleOut(), containsString("[go] setting environment variable 'GO_SERVER_URL' to value 'somewhere-does-not-matter'\n"));
assertThat(artifactManipulator.consoleOut(), containsString("[go] setting environment variable 'GO_TRIGGER_USER' to value ''\n"));
assertThat(artifactManipulator.consoleOut(), containsString("[go] setting environment variable 'GO_PIPELINE_NAME' to value 'pipeline1'\n"));
assertThat(artifactManipulator.consoleOut(), containsString("[go] setting environment variable 'GO_PIPELINE_COUNTER' to value '-2'\n"));
assertThat(artifactManipulator.consoleOut(), containsString("[go] setting environment variable 'GO_PIPELINE_LABEL' to value '100'\n"));
assertThat(artifactManipulator.consoleOut(), containsString("[go] setting environment variable 'GO_STAGE_NAME' to value 'mingle'\n"));
assertThat(artifactManipulator.consoleOut(), containsString("[go] setting environment variable 'GO_STAGE_COUNTER' to value '100'\n"));
assertThat(artifactManipulator.consoleOut(), containsString("[go] setting environment variable 'GO_JOB_NAME' to value 'run-ant'"));
}

private void createDummyFilesAndDirectories(File workingdir) {
for (int i = 0; i < 2; i++) {
File directory = new File(workingdir.getPath() + "/dir" + i);
Expand Down

3 comments on commit 9d03d3e

@zabil
Copy link

@zabil zabil commented on 9d03d3e Feb 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change this to

        assertThat(artifactManipulator.consoleOut(), containsString("[go] setting environment variable 'GO_SERVER_URL' to value 'somewhere-does-not-matter'\n"
                + "[go] setting environment variable 'GO_TRIGGER_USER' to value ''\n"
                + "[go] setting environment variable 'GO_PIPELINE_NAME' to value 'pipeline1'\n"
                + "[go] setting environment variable 'GO_PIPELINE_COUNTER' to value '-2'\n"
                + "[go] setting environment variable 'GO_PIPELINE_LABEL' to value '100'\n"
        + "[go] setting environment variable 'GO_STAGE_NAME' to value 'mingle'\n"
        + "[go] setting environment variable 'GO_STAGE_COUNTER' to value '100'\n"
        + "[go] setting environment variable 'GO_JOB_NAME' to value 'run-ant'"));

because we need to care about the order of reporting and don't want people to mess with that.

@srinivasupadhya
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have incorporated the first one. For the second bit - verify order of env. vars, I think DefaultGoPublisherTest is doing that.

@arvindsv
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the order being tested in the other test. If it is, then alright. If not, I think what @zabil says makes sense. I mean, I know verifyConsoleOutput verifies the lines after join, but there's no specific test for the order (if the order is a property of the output, "GO_SERVER_USER should be before GO_TRIGGER_USER, which should be before ...").

Please sign in to comment.