-
Notifications
You must be signed in to change notification settings - Fork 44
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.
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
NCL-6053: pig - .bacon dir in a temp loc #459
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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"; | ||
|
@@ -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!"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) { | ||
|
There was a problem hiding this comment.
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 ofRuntime.exec()
tonull
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 andrelease
with another project using the same.bacon/
directory, and what would happen then?I kind of like
$HOME/.bacon/<project_id>/
or something similar.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?