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

michalszynkiewicz · 2020-09-12T19:38:45Z

Checklist:

Have you added a note in the CHANGELOG wiki for your change if user-facing?
Have you added unit tests for your change?

codecov · 2020-09-12T19:43:06Z

Codecov Report

Merging #459 (e4a8953) into master (0926f6a) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff            @@
##             master    #459   +/-   ##
========================================
  Coverage      5.48%   5.48%           
  Complexity      103     103           
========================================
  Files           133     133           
  Lines          4994    4994           
  Branches        463     463           
========================================
  Hits            274     274           
  Misses         4701    4701           
  Partials         19      19

Impacted Files	Coverage Δ	Complexity Δ
.../java/org/jboss/pnc/bacon/pig/impl/PigContext.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0926f6a...e4a8953. Read the comment docs.

dwalluck · 2020-09-13T16:14:05Z

integration-tests/src/test/java/org/jboss/pnc/bacon/test/AbstractTest.java

        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);


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.

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

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)

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

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?

rnc · 2020-09-13T17:44:44Z

What is the bacon dir used for? Is it needed by users after a run?

dwalluck · 2020-09-13T17:56:28Z

What is the bacon dir used for? Is it needed by users after a run?

I believe it stores the state for a particular run and is then used when you do a release.

But, that makes it specific to a particular run, and you won't know without examining the file which run this belongs to. It is also a hidden directory and created in whatever directory you happen to be in when you run it. Obviously, if it's a read-only directory for your user it's just going to fail. It doesn't create it where it's known to be writable.

michalszynkiewicz · 2020-09-13T20:48:04Z

pig context stores data between executions of pig.
With it one can do pig configure and then do pig build and data from one would be passed to another. As written in the review comment, it's invalidated if configuration has been changed between the executions.

dwalluck · 2020-09-13T20:59:17Z

What data specifically can be shared between runs? In any case, a change in project should cause the configuration to be invalidated. It seems easier to me to just store the configuration under some unique key derived from the project name or something else.

I noticed also having to set the pnc configuration file via an environment variable. This is a bit odd and not usually how other programs work. I'd expect to set it via a program flag, if anything, as the defaults should also be reasonable to begin with.

rnc · 2020-09-14T13:28:08Z

@michalszynkiewicz Interesting, thanks. How does it know whether to invalidate it or not? I wonder if it would be better to have this as an option flag - it could default to , as @dwalluck suggested $Home/.bacon but can be overridden to use a current directory. This would solve the problem with these hidden directories being left lying around, would still allow the same flexibility and the tests could use it with a temp dir.

Also... is this documented?!

michalszynkiewicz · 2020-09-14T18:39:49Z

there's a --clean option that invalidates the context too.
I like the current working directory better but we can create a JIRA and discuss it. It is not really relevant to this PR ;)

Re using env variable for the location, I think it's better than system property or parameter because you can set it in your .bashrc or .profile and don't have to pass it every time.
It's also not an uncommon thing in the java world, e.g. spring profile can be selected this way, configuration of apps running in kubernetes pods is also often passed this way.

michalszynkiewicz · 2020-09-14T18:40:28Z

Regarding how does it know, I calculate a hash of the pig configuration directory.

rnc · 2020-09-15T05:57:17Z

integration-tests/src/test/java/org/jboss/pnc/bacon/test/pig/PigFunctionalTest.java

@@ -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!");


FatalException extends runtimeexception - why the change here?

rnc · 2020-09-15T06:20:09Z

integration-tests/src/test/java/org/jboss/pnc/bacon/test/pig/PigFunctionalTest.java

        }
+        System.setProperty(PIG_CONTEXT_DIR, targetDirectory.toString());


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

rnc · 2020-09-15T06:24:07Z

pig/src/main/java/org/jboss/pnc/bacon/pig/impl/PigContext.java

@@ -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));


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.

rnc · 2020-09-15T06:25:24Z

there's a --clean option that invalidates the context too.
I like the current working directory better but we can create a JIRA and discuss it. It is not really relevant to this PR ;)

Re using env variable for the location, I think it's better than system property or parameter because you can set it in your .bashrc or .profile and don't have to pass it every time.
It's also not an uncommon thing in the java world, e.g. spring profile can be selected this way, configuration of apps running in kubernetes pods is also often passed this way.

Could you create a separate JIRA then please? As mention in one of my comments, picocli supports parameters with environment variables I think - that also could be a separate ticket perhaps?

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

e4a8953

dwalluck reviewed Sep 13, 2020

View reviewed changes

rnc reviewed Sep 15, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

michalszynkiewicz commented Sep 12, 2020

codecov bot commented Sep 12, 2020 •

edited

Loading

dwalluck Sep 13, 2020

rnc Sep 13, 2020

michalszynkiewicz Sep 13, 2020

rnc Sep 15, 2020

rnc Sep 15, 2020

rnc commented Sep 13, 2020

dwalluck commented Sep 13, 2020

michalszynkiewicz commented Sep 13, 2020

dwalluck commented Sep 13, 2020

rnc commented Sep 14, 2020

michalszynkiewicz commented Sep 14, 2020

michalszynkiewicz commented Sep 14, 2020

rnc Sep 15, 2020

rnc Sep 15, 2020

rnc Sep 15, 2020

rnc commented Sep 15, 2020

		}
		System.setProperty(PIG_CONTEXT_DIR, targetDirectory.toString());

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

Are you sure you want to change the base?

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

Conversation

michalszynkiewicz commented Sep 12, 2020

Checklist:

codecov bot commented Sep 12, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnc commented Sep 13, 2020

dwalluck commented Sep 13, 2020

michalszynkiewicz commented Sep 13, 2020

dwalluck commented Sep 13, 2020

rnc commented Sep 14, 2020

michalszynkiewicz commented Sep 14, 2020

michalszynkiewicz commented Sep 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnc commented Sep 15, 2020

codecov bot commented Sep 12, 2020 •

edited

Loading