Skip to content
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.

Sign up for GitHub

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

Jump to bottom

NCL-6053: pig - .bacon dir in a temp loc #459

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

NCL-6053: pig - .bacon dir in a temp loc #459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions integration-tests/src/test/java/org/jboss/pnc/bacon/test/AbstractTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.nio.file.Path;
import java.util.Base64;
import java.util.Random;

Expand Down Expand Up @@ -64,28 +65,33 @@ protected static String getRandomString() {
}

protected void execute(String... args) {
ExecutionResult result = executor.runCommand(args);
ExecutionResult result = executor.runCommand(null, args);
assertThat(result.getOutput()).isEmpty();
assertThat(result.getError()).isEmpty();
assertThat(result.getRetval()).isZero();
}

protected <T> T executeAndDeserialize(Class<T> clazz, String... args) throws JsonProcessingException {
ExecutionResult result = executor.runCommand(args);
ExecutionResult result = executor.runCommand(null, args);
log.debug("stderr:{}{}", System.lineSeparator(), result.getError());
assertThat(result.getRetval()).isZero();
return result.fromYAML(clazz);
}

protected <T> T executeAndDeserializeJSON(Class<T> clazz, String... args) throws JsonProcessingException {
ExecutionResult result = executor.runCommand(args);
protected <T> T executeAndDeserializeJSON(Class<T> clazz, Path workingDir, String... args)
throws JsonProcessingException {
ExecutionResult result = executor.runCommand(workingDir, args);
log.debug("stderr:{}{}", System.lineSeparator(), result.getError());
assertThat(result.getRetval()).isZero();
return result.fromYAML(clazz);
}

protected ExecutionResult executeAndGetResult(String... args) {
return executor.runCommand(args);
return executor.runCommand(null, args);
Copy link
Contributor

Choose a reason for hiding this comment

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

So this sets the dir argument of Runtime.exec() to null which means $CWD, but I still don't really like that it will create this dir anywhere I run it. I think picking a single location would be better.

The other issue is because it's not a unique location, it would be possible to run the run with one project and release with another project using the same .bacon/ directory, and what would happen then?

I kind of like $HOME/.bacon/<project_id>/ or something similar.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great points - I think we need to understand its purpose before choosing a solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By default the .bacon directory it's created where the program is executed. The location can be overriden with an environment varialbe and a system property.
Current directory was chosen as a default to allow running the tool for multiple projects separately.

If the config yaml or any other file in the config dir would change, the context will be dropped (the previous one won't be read and will be overwritten at the end of the execution). It's checked by storing a hash sum of all files in the directory (and their locations, moving a file is also considered a change).

executeAndGetResult is also used for non-pig tests, for it I haven't changed the working directory (hence the null here)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to use a Junit tmp dir here and pass that through?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I've just seen down below you are passing a TempDir in :-) What about changing for all tests so they consistently run from a tempDirectory - or do some require a certain context pwd?

}

protected ExecutionResult executeAndGetResult(Path workingDir, String... args) {
return executor.runCommand(workingDir, args);
}

}
8 changes: 5 additions & 3 deletions integration-tests/src/test/java/org/jboss/pnc/bacon/test/CLIExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import org.jboss.pnc.bacon.common.Constant;

import java.io.BufferedReader;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
Expand All @@ -22,9 +23,9 @@
*/
public class CLIExecutor {
private static final Path BACON_JAR = Paths.get("..", "cli", "target", "bacon.jar").toAbsolutePath().normalize();
public static final Path CONFIG_LOCATION = Paths.get("target", "test-config");
public static final Path CONFIG_LOCATION = Paths.get("target", "test-config").toAbsolutePath().normalize();

public ExecutionResult runCommand(String... args) {
public ExecutionResult runCommand(Path workingDirectory, String... args) {
try {
checkExecutable();

Expand All @@ -38,7 +39,8 @@ public ExecutionResult runCommand(String... args) {
System.out.println(
"Running command: " + Arrays.stream(cmdarray).collect(Collectors.joining("' '", "'", "'"))
+ "\n\twith env " + Arrays.toString(env));
Process process = Runtime.getRuntime().exec(cmdarray, env);
File workingDirectoryFile = workingDirectory != null ? workingDirectory.toFile() : null;
Process process = Runtime.getRuntime().exec(cmdarray, env, workingDirectoryFile);

CompletableFuture<String> output = CompletableFuture
.supplyAsync(() -> readInputStream(process.getInputStream()));
Expand Down
7 changes: 5 additions & 2 deletions integration-tests/src/test/java/org/jboss/pnc/bacon/test/pig/PigFunctionalTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import org.apache.commons.lang3.RandomStringUtils;
import org.jboss.pnc.bacon.common.Constant;
import org.jboss.pnc.bacon.common.exception.FatalException;
import org.jboss.pnc.bacon.config.Config;
import org.jboss.pnc.bacon.config.PigConfig;
import org.jboss.pnc.bacon.config.Validate;
Expand All @@ -15,6 +14,8 @@
import java.nio.file.Path;
import java.util.Optional;

import static org.jboss.pnc.bacon.common.Constant.PIG_CONTEXT_DIR;

@Tag(TestType.REAL_SERVICE_ONLY)
public abstract class PigFunctionalTest {
static final String emptyNameBase1 = "michalszynkiewicz-et-%s";
Expand All @@ -29,13 +30,15 @@ static String init(Path configDir, boolean clean, Optional<String> releaseStorag
String suffix = prepareSuffix();
// todo release storage url mocking
if (configDir == null) {
throw new FatalException("You need to specify the configuration directory!");
throw new RuntimeException("You need to specify the configuration directory!");
Copy link
Contributor

Choose a reason for hiding this comment

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

FatalException extends runtimeexception - why the change here?

}
System.setProperty(PIG_CONTEXT_DIR, targetDirectory.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think tests that set system properties should use https://github.com/stefanbirkner/system-lambda (https://github.com/stefanbirkner/system-rules for JUnit4)

// validate the PiG config
PigConfig pig = Config.instance().getActiveProfile().getPig();
if (pig == null) {
throw new Validate.ConfigMissingException("Pig configuration missing");
}

pig.validate();

PigContext.init(clean, configDir, targetDirectory.toAbsolutePath().toString(), releaseStorageUrl);
Expand Down
5 changes: 3 additions & 2 deletions integration-tests/src/test/java/org/jboss/pnc/bacon/test/pig/PigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
import org.junit.jupiter.api.TestMethodOrder;
import org.junit.jupiter.api.io.TempDir;

import java.io.IOException;
import java.nio.file.Files;
Expand Down Expand Up @@ -68,7 +69,7 @@ class PigTest extends AbstractTest {

@Test
@Order(1)
void shouldCreateProduct() throws IOException {
void shouldCreateProduct(@TempDir Path workingDir) throws IOException {
final Path configFile = CONFIG_LOCATION;
replaceSuffixInConfigFile(configFile.resolve("build-config.yaml"));

Expand Down Expand Up @@ -143,7 +144,7 @@ void shouldCreateProduct() throws IOException {
.post(GROUP_CONFIG_BUILD_CONFIGS.apply(UNIVERSAL_ID))
.then()
.getEntity(GROUP_CONFIG, groupConfigWithBuildConfig);
ExecutionResult output = executeAndGetResult("pig", "configure", configFile.toString());
ExecutionResult output = executeAndGetResult(workingDir, "pig", "configure", configFile.toString());
assertThat(output.getOutput()).contains("name: \"Product Foobar " + SUFFIX + "\"");
}

Expand Down
2 changes: 1 addition & 1 deletion pig/src/main/java/org/jboss/pnc/bacon/pig/impl/PigContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ private static PigContext readContext(boolean clean, Path configDir) {
String sha = hashDirectory(configDir);

PigContext result;
String ctxLocationEnv = System.getenv(PIG_CONTEXT_DIR);
String ctxLocationEnv = System.getProperty(PIG_CONTEXT_DIR, System.getenv(PIG_CONTEXT_DIR));
Copy link
Contributor

Choose a reason for hiding this comment

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

For reference, for perhaps a future consideration, picocli supports default values of e.g. environment variables ( https://picocli.info/#_variable_interpolation ) so it could be a parameter as well.

Path contextDir = ctxLocationEnv == null ? Paths.get(".bacon") : Paths.get(ctxLocationEnv);
Path contextJson = contextDir.resolve("pig-context.json");
if (!clean && Files.exists(contextJson)) {
Expand Down