> On July 2, 2018, 12:46 p.m., Peter Bacsko wrote: > > core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java > > Lines 95 (patched) > > <https://reviews.apache.org/r/59620/diff/7/?file=2046211#file2046211line95> > > > > Is this package private for a reason?
No, simply omission. Thanks for finding this! > On July 2, 2018, 12:46 p.m., Peter Bacsko wrote: > > docs/src/site/twiki/WorkflowFunctionalSpec.twiki > > Lines 1667-1669 (patched) > > <https://reviews.apache.org/r/59620/diff/7/?file=2046214#file2046214line1667> > > > > Do we really need to access a different cluster other than the one > > we're running on? > > > > If key is located on a separate cluster which is secure we won't be > > allowed to access that file without proper credentials. Basically it's the > > same problem that appears under disctp. It's really not self-explanatory to > > setup cross-cluster authentication. > > > > Therefore I'm against it. Also, the name node is available from > > "fs.defaultFS" property. I could certainly see someone storing a credential on a secure object store (e.g. S3-like FS). > On July 2, 2018, 12:46 p.m., Peter Bacsko wrote: > > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java > > Lines 41 (patched) > > <https://reviews.apache.org/r/59620/diff/7/?file=2046219#file2046219line41> > > > > We don't use XLog inside LauncherMain, only Sysout. This will change in > > the future, but for now, let's stick to the conventions that we have > > already. I have left the help methods to leave logging level intention in place but removed all references to XLog. Would you prefer the helpers to go too? > On July 2, 2018, 12:46 p.m., Peter Bacsko wrote: > > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java > > Lines 166 (patched) > > <https://reviews.apache.org/r/59620/diff/7/?file=2046219#file2046219line166> > > > > A better method name would be something like isValidUri() Sounds good > On July 2, 2018, 12:46 p.m., Peter Bacsko wrote: > > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java > > Lines 185-188 (patched) > > <https://reviews.apache.org/r/59620/diff/7/?file=2046219#file2046219line185> > > > > Is it necessary to validate these separately? If anything, these values > > should be checked inside Oozie, not when the container is alreay running. I have removed these checks. > On July 2, 2018, 12:46 p.m., Peter Bacsko wrote: > > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java > > Lines 201-204 (patched) > > <https://reviews.apache.org/r/59620/diff/7/?file=2046219#file2046219line201> > > > > If we don't allow relative URIs (null scheme), then this check should > > be placed inside validUri(). Integrated into isValidURI. > On July 2, 2018, 12:46 p.m., Peter Bacsko wrote: > > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java > > Lines 82-99 (patched) > > <https://reviews.apache.org/r/59620/diff/7/?file=2046220#file2046220line82> > > > > We should not clone repositories under an unit test. I suggest using a > > Jsch mock for testing. > > > > There are different ways to achieve this. I can think of having a > > property like "oozie.git.operations.class" which defaults to this class but > > we can override it to a different implementation which does not use JSch. I have found a few descriptions on how to build a unit-test server in JGit; now doing that. > On July 2, 2018, 12:46 p.m., Peter Bacsko wrote: > > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java > > Lines 43 (patched) > > <https://reviews.apache.org/r/59620/diff/7/?file=2046223#file2046223line43> > > > > As I mentioned above, let's avoid cloning a repository during test > > execution. Still clones but from an in-JVM server. > On July 2, 2018, 12:46 p.m., Peter Bacsko wrote: > > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java > > Lines 119-122 (patched) > > <https://reviews.apache.org/r/59620/diff/7/?file=2046223#file2046223line119> > > > > Three things: > > > > 1. We don't need JobConf anymore because we don't launch the action as > > a MapReduce action. Just use the plain Configuration class which JobConf > > extends. > > > > 2. No need to set "mapred.job.tracker", it's a deprecated property > > anyway > > > > 3. createJobClient() creates an MR job client, but we don't need it, > > plus the return value is ignored. I still use createJobConf from XTestCase to keep uniformity with other test cases but I use a Configuration instead of a JobConf now. - Clay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59620/#review205636 ----------------------------------------------------------- On June 26, 2018, 9:50 p.m., Clay B. wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59620/ > ----------------------------------------------------------- > > (Updated June 26, 2018, 9:50 p.m.) > > > Review request for oozie and András Piros. > > > Bugs: OOZIE-2877 > https://issues.apache.org/jira/browse/OOZIE-2877 > > > Repository: oozie-git > > > Description > ------- > > OOZIE-2877 - Oozie Git Action > > > Diffs > ----- > > client/src/main/resources/git-action-1.0.xsd PRE-CREATION > core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java > PRE-CREATION > core/src/main/resources/oozie-default.xml ff1820c > core/src/test/java/org/apache/oozie/test/XTestCase.java 661970d > docs/src/site/twiki/WorkflowFunctionalSpec.twiki 76cbe21 > examples/src/main/apps/git/job.properties PRE-CREATION > examples/src/main/apps/git/workflow.xml PRE-CREATION > pom.xml 0c39d64 > sharelib/git/pom.xml PRE-CREATION > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java > PRE-CREATION > > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java > PRE-CREATION > > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java > PRE-CREATION > > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMainGetKey.java > PRE-CREATION > > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java > PRE-CREATION > sharelib/pom.xml 6a0864d > src/main/assemblies/sharelib.xml 07dc69c > webapp/pom.xml 4a32b54 > > > Diff: https://reviews.apache.org/r/59620/diff/7/ > > > Testing > ------- > > Tested using unit and integration tests. Still need to: > * Test on a cluster > * Test with an authenticated SSH hosted Git repo > > Sumitted a request to the JGit community as their branch pulling code seems > to have an > [issue](https://dev.eclipse.org/mhonarc/lists/jgit-dev/msg03343.html). > > Andras has tested against his patch 011 with results at > https://issues.apache.org/jira/browse/OOZIE-2877?focusedCommentId=16459523&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16459523 > > > File Attachments > ---------------- > > 0001-OOZIE-2877-Oozie-Git-Action.patch > > https://reviews.apache.org/media/uploaded/files/2017/05/29/24f90a78-3dc1-49fe-bf29-5927a3cd5e72__0001-OOZIE-2877-Oozie-Git-Action.patch > Patch > > https://reviews.apache.org/media/uploaded/files/2017/05/29/dd23dd72-67e0-456f-9b52-e566d8d17d16__0001-OOZIE-2877-Oozie-Git-Action.patch > > > Thanks, > > Clay B. > >
