Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[András Piros via Review Board]

> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
> > Lines 60 (patched)
> > 
> >
> > Are system props reset after this test run?
> 
> Clay B. wrote:
> I am not sure what the entire chain of `ActionExecutorTestCase`'s methods 
> are doing. However, I am not explicitly clearing this to my knowledge; do you 
> expect issues? (For reference, it seems 
> [`TestJavaActionExecutor()`](https://github.com/apache/oozie/blob/5998c18fde1da769e91e3ef1bcca484723730c76/core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java#L90)
>  is not clearing these either?)
> 
> András Piros wrote:
> I'd rather not set any system properties here but go for 
> [`Services.get().getConf().set()`](https://github.com/apache/oozie/blob/eb168360c550943828d9fe2c7bfdcd4f5e830003/core/src/test/java/org/apache/oozie/service/TestActionService.java#L52-L53),
>  if that works.
> 
> If not, I'd still set this system property in `@Before void setUp()` and 
> reset in `@After void tearDown()` as this one is used by other important 
> pieces of code.
> 
> Clay B. wrote:
> It seems this idiom is followed by the following and I am missing where 
> they do clean-up?
> 
> * 
> [Pig](https://github.com/apache/oozie/blob/master/sharelib/pig/src/test/java/org/apache/oozie/action/hadoop/TestPigActionExecutor.java#L81-L85)
> * 
> [Spark](https://github.com/apache/oozie/blob/299370b44e2d7b22bfe622d19f65b02c5b18282f/sharelib/spark/src/test/java/org/apache/oozie/action/hadoop/TestSparkActionExecutor.java#L66-L69)
> * 
> [MapReduce](https://github.com/apache/oozie/blob/5e1c9d362afe1b2c6423a386aeac7f04d3337f65/sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java#L86-L90)

Thanks for the explanation, dropping this issue.


- András


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review176291
---


On Aug. 3, 2018, 10 p.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated Aug. 3, 2018, 10 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 b69d2c9 
>   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 
>   fluent-job/fluent-job-api/pom.xml 4c9b853 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
>  ec56554 
>   fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e 
>   fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890 
>   pom.xml 92358aa 
>   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/GitServer.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/TestGitMain.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 fd3f89f 
> 
> 
> Diff: 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[András Piros via Review Board]

> On April 30, 2018, 3:12 p.m., Peter Cseh wrote:
> > client/src/main/resources/git-action-0.1.xsd
> > Lines 32 (patched)
> > 
> >
> > I don't know if it's possible to define uris outside 
> > oozie.service.HadoopAccessorService.nameNode.whitelist here.
> > 
> > Can you please add a test for that?
> 
> Clay B. wrote:
> Thanks Peter for thinking through this. I do not know that it be 
> necessary we allow URI's other than the HadoopAccessorService whitelisted 
> nameservices? Andras had a request that I rename `cloneRepoToHdfs` to be less 
> opinionated but I presumed it would still be a whitelisted HCFS of some sort. 
> For a test here, are you thinking of me using a `file://` path for 
> `testWhenRepoIsClonedThenGitIndexContentIsReadSuccessfully()` in 
> `TestIntegrationGitActionExecutor.java` or can you provide me a bit more 
> guidance or the use-case?

@Peter Cseh it's already covered by `HadoopAccessorService#createFileSystem()` 
that to latter end is called by `JavaActionExecutor#setupActionConf()` as well. 
So no need to cover that separately.


> On April 30, 2018, 3:12 p.m., Peter Cseh wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 137-145 (patched)
> > 
> >
> > I wonder how strict we should be with credential files. We might want 
> > to do a ssh-like check. Ssh fails if your .pem file is not readably only by 
> > your user, that's why we're setting the permissions here.
> > 
> > At least we could print out a warning to help users avoid leaking 
> > credentials to everyone from HDFS
> > 
> > Or we can go ssh-level strict and have an option to do fall back to 
> > warnings only.
> 
> Clay B. wrote:
> Good idea! Indeed, I've had a hard time getting users to "do the right 
> thing" in the past. As you see many more environments than I do, would you 
> recommend we also check HDFS extended ACLs as well if available? Would you 
> want an "allow insecure credential" boolean or would you want something more 
> for the override?

@Peter Cseh can you please give an answer? Thanks!


- András


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review202124
---


On Aug. 3, 2018, 10 p.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated Aug. 3, 2018, 10 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 b69d2c9 
>   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 
>   fluent-job/fluent-job-api/pom.xml 4c9b853 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
>  ec56554 
>   fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e 
>   fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890 
>   pom.xml 92358aa 
>   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/GitServer.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/TestGitMain.java 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[András Piros via Review Board]

> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
> > Lines 182-183 (patched)
> > 
> >
> > These nested ifs can be merged.

Done.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 200-201 (patched)
> > 
> >
> > Do we need this?

Removed.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java
> > Lines 92 (patched)
> > 
> >
> > Typo: "Checkiging"

Fixed.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
> > Lines 114 (patched)
> > 
> >
> > Better error message would be "Expected ActionExecutorException"

Done.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
> > Lines 117 (patched)
> > 
> >
> > Better error message would be "Unexpected exception message" or sth 
> > like that

Done.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
> > Lines 148-154 (patched)
> > 
> >
> > What are we testing here exactly?
> > 
> > I can't see any assert() calls.

Done.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
> > Lines 183-188 (patched)
> > 
> >
> > Same here

Done.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java
> > Lines 101-104 (patched)
> > 
> >
> > This should be replaced with a waitFor() method call - see Oozie 
> > codebase for examples.

Done.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
> > Lines 30 (patched)
> > 
> >
> > Unused import

Done.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
> > Lines 86 (patched)
> > 
> >
> > readContent variable is unused

Done.


- András


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review206939
---


On Aug. 3, 2018, 10 p.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated Aug. 3, 2018, 10 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 b69d2c9 
>   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 
>   fluent-job/fluent-job-api/pom.xml 4c9b853 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[András Piros via Review Board]

> On June 5, 2018, 4:46 p.m., Robert Kanter wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 216 (patched)
> > 
> >
> > Is this necessary?  Why not simply throw whatever exception 
> > (IOException, IllegalArgumentException, etc)?  We don't seem to be doing 
> > any special handling or anything for GitMainExceptions.
> 
> Clay B. wrote:
> I was simply copying the idiom from JavaMain but do not have any broader 
> plan for the exception; the same is true for `GitOperationsException`.
> 
> Robert Kanter wrote:
> I forget exactly the reason off the top of my head, but we needed to wrap 
> Exceptions in another layer in the Java Action to handle a certain case.  I 
> believe it was related to the fact that we'd be running unknown Java code 
> (provided by the user), but I'm not sure.  If you look at LauncherAM, it's 
> actually doing something with JavaMainException.  Anyway, you can see that 
> none of the other actions do this, so I don't think we need a 
> GitMainException.

Removed `GitMainException`.


- András


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review204323
---


On Aug. 3, 2018, 10 p.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated Aug. 3, 2018, 10 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 b69d2c9 
>   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 
>   fluent-job/fluent-job-api/pom.xml 4c9b853 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
>  ec56554 
>   fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e 
>   fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890 
>   pom.xml 92358aa 
>   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/GitServer.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/TestGitMain.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 fd3f89f 
> 
> 
> Diff: https://reviews.apache.org/r/59620/diff/10/
> 
> 
> 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=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
>   
> 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[András Piros via Review Board]

> On Aug. 8, 2018, 9:57 a.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
> > Lines 152 (patched)
> > 
> >
> > Better naming suggested: ActionConfVerifier

Done.


> On Aug. 8, 2018, 9:57 a.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 38 (patched)
> > 
> >
> > General thoughts: this class has quite a few protected methods. We have 
> > to think about whether it's necessary or not. If the class is not going to 
> > be subclassed, making them "private" is preferable.

Done.


> On Aug. 8, 2018, 9:57 a.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 43 (patched)
> > 
> >
> > I'd prefer this as being private, with having a package private setter 
> > method.

Done.


> On Aug. 8, 2018, 9:57 a.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java
> > Lines 125 (patched)
> > 
> >
> > This string can be placed directly in the constructor.

Done.


> On Aug. 8, 2018, 9:57 a.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/GitServer.java
> > Lines 74 (patched)
> > 
> >
> > At least a warning/error message would be good here.

Done.


- András


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review206974
---


On Aug. 3, 2018, 10 p.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated Aug. 3, 2018, 10 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 b69d2c9 
>   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 
>   fluent-job/fluent-job-api/pom.xml 4c9b853 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
>  ec56554 
>   fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e 
>   fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890 
>   pom.xml 92358aa 
>   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/GitServer.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/TestGitMain.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 fd3f89f 
> 
> 
> Diff: https://reviews.apache.org/r/59620/diff/10/
> 
> 
> 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 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[András Piros via Review Board]

> On July 2, 2018, 12:46 p.m., Peter Bacsko wrote:
> > docs/src/site/twiki/WorkflowFunctionalSpec.twiki
> > Lines 1667-1669 (patched)
> > 
> >
> > 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.
> 
> Clay B. wrote:
> I could certainly see someone storing a credential on a secure object 
> store (e.g. S3-like FS).
> 
> Peter Bacsko wrote:
> In that case, we have to mention in the docs that access control on the 
> remote side must be properly configured. Eg. if it's a hdfs:// URL that 
> points to an non-secure cluster, appropriate HDFS permissions are still 
> necessary.

A paragraph added to the docs.


> 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)
> > 
> >
> > 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.
> 
> Clay B. wrote:
> 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?
> 
> Peter Bacsko wrote:
> We haven't settled down on a logging solution. Geza suggested JDK loggers 
> (so that they don't conflict with log4j/log4j2/logback, whatever we migrate 
> to), but it's not finalized yet.

`XLog` references are not more present in `GitMain`.


- András


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review205636
---


On Aug. 3, 2018, 10 p.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated Aug. 3, 2018, 10 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 b69d2c9 
>   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 
>   fluent-job/fluent-job-api/pom.xml 4c9b853 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
>  ec56554 
>   fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e 
>   fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890 
>   pom.xml 92358aa 
>   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/GitServer.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/TestGitMain.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 fd3f89f 
> 
> 
> Diff: 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Peter Bacsko via Review Board]

> On júl. 2, 2018, 12:46 du, Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 41 (patched)
> > 
> >
> > 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.
> 
> Clay B. wrote:
> 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?

We haven't settled down on a logging solution. Geza suggested JDK loggers (so 
that they don't conflict with log4j/log4j2/logback, whatever we migrate to), 
but it's not finalized yet.


- Peter


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review205636
---


On aug. 3, 2018, 10 du, Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated aug. 3, 2018, 10 du)
> 
> 
> 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 b69d2c9 
>   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 
>   fluent-job/fluent-job-api/pom.xml 4c9b853 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
>  ec56554 
>   fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e 
>   fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890 
>   pom.xml 92358aa 
>   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/GitServer.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/TestGitMain.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 fd3f89f 
> 
> 
> Diff: https://reviews.apache.org/r/59620/diff/10/
> 
> 
> 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=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.
> 
>

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Peter Bacsko via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review206974
---




core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 152 (patched)


Better naming suggested: ActionConfVerifier



core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 169 (patched)


Check visibility of this class & methods (package private OK?)



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 38 (patched)


General thoughts: this class has quite a few protected methods. We have to 
think about whether it's necessary or not. If the class is not going to be 
subclassed, making them "private" is preferable.



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 43 (patched)


I'd prefer this as being private, with having a package private setter 
method.



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java
Lines 125 (patched)


This string can be placed directly in the constructor.



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/GitServer.java
Lines 74 (patched)


At least a warning/error message would be good here.


- Peter Bacsko


On aug. 3, 2018, 10 du, Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated aug. 3, 2018, 10 du)
> 
> 
> 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 b69d2c9 
>   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 
>   fluent-job/fluent-job-api/pom.xml 4c9b853 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
>  ec56554 
>   fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e 
>   fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890 
>   pom.xml 92358aa 
>   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/GitServer.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/TestGitMain.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 fd3f89f 
> 
> 
> Diff: https://reviews.apache.org/r/59620/diff/10/
> 
> 
> 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 
> 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Peter Bacsko via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review206939
---




core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 182-183 (patched)


These nested ifs can be merged.



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 200-201 (patched)


Do we need this?



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java
Lines 92 (patched)


Typo: "Checkiging"



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/GitServer.java
Lines 52 (patched)


This solution is much better than the original.

Just one more thing: perhaps instead of hard-coding the port 9418, we could 
pick one dynamically. Or at least check if it's not occupied by some other 
process.



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
Lines 57 (patched)


I'm just curious - how will Git know that it has to connect to the simple 
local server instead of Github?



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
Lines 114 (patched)


Better error message would be "Expected ActionExecutorException"



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
Lines 117 (patched)


Better error message would be "Unexpected exception message" or sth like 
that



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
Lines 148-154 (patched)


What are we testing here exactly?

I can't see any assert() calls.



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
Lines 183-188 (patched)


Same here



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java
Lines 101-104 (patched)


This should be replaced with a waitFor() method call - see Oozie codebase 
for examples.



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
Lines 30 (patched)


Unused import



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
Lines 86 (patched)


readContent variable is unused


- Peter Bacsko


On aug. 3, 2018, 10 du, Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated aug. 3, 2018, 10 du)
> 
> 
> 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 b69d2c9 
>   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 
>   fluent-job/fluent-job-api/pom.xml 4c9b853 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
>  ec56554 
>   fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e 
>   fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890 
>   pom.xml 92358aa 
>   sharelib/git/pom.xml PRE-CREATION 
>   

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Peter Bacsko via Review Board]

> On júl. 2, 2018, 12:46 du, Peter Bacsko wrote:
> > docs/src/site/twiki/WorkflowFunctionalSpec.twiki
> > Lines 1667-1669 (patched)
> > 
> >
> > 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.
> 
> Clay B. wrote:
> I could certainly see someone storing a credential on a secure object 
> store (e.g. S3-like FS).

In that case, we have to mention in the docs that access control on the remote 
side must be properly configured. Eg. if it's a hdfs:// URL that points to an 
non-secure cluster, appropriate HDFS permissions are still necessary.


- Peter


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review205636
---


On aug. 3, 2018, 10 du, Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated aug. 3, 2018, 10 du)
> 
> 
> 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 b69d2c9 
>   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 
>   fluent-job/fluent-job-api/pom.xml 4c9b853 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
>  ec56554 
>   fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e 
>   fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890 
>   pom.xml 92358aa 
>   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/GitServer.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/TestGitMain.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 fd3f89f 
> 
> 
> Diff: https://reviews.apache.org/r/59620/diff/10/
> 
> 
> 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=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
>   
> 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/
---

(Updated Aug. 3, 2018, 10 p.m.)


Review request for oozie and András Piros.


Changes
---

This patch addresses a number of JavaDoc issues. There are still a ton of 
`@throws` complaints; is it best to document unchecked exceptions?


Bugs: OOZIE-2877
https://issues.apache.org/jira/browse/OOZIE-2877


Repository: oozie-git


Description
---

OOZIE-2877 - Oozie Git Action


Diffs (updated)
-

  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 b69d2c9 
  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 
  fluent-job/fluent-job-api/pom.xml 4c9b853 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
 7bb82e5 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
 ec56554 
  fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e 
  fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890 
  pom.xml 92358aa 
  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/GitServer.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/TestGitMain.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 fd3f89f 


Diff: https://reviews.apache.org/r/59620/diff/10/

Changes: https://reviews.apache.org/r/59620/diff/9-10/


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=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.

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/
---

(Updated Aug. 3, 2018, 8:24 p.m.)


Review request for oozie and András Piros.


Changes
---

This provides a unit-test Git server, Fluent API support and checks recent 
changes to isValidUri() among other requests for change.


Bugs: OOZIE-2877
https://issues.apache.org/jira/browse/OOZIE-2877


Repository: oozie-git


Description
---

OOZIE-2877 - Oozie Git Action


Diffs (updated)
-

  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 
  fluent-job/fluent-job-api/pom.xml 4c9b853 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
 7bb82e5 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
 ec56554 
  fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e 
  fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890 
  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/GitServer.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/TestGitMain.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/9/

Changes: https://reviews.apache.org/r/59620/diff/8-9/


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=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.

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/
---

(Updated July 30, 2018, 12:56 a.m.)


Review request for oozie and András Piros.


Changes
---

Matches patch 14 on the JIRA


Bugs: OOZIE-2877
https://issues.apache.org/jira/browse/OOZIE-2877


Repository: oozie-git


Description
---

OOZIE-2877 - Oozie Git Action


Diffs (updated)
-

  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/GitServer.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/TestGitMain.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/8/

Changes: https://reviews.apache.org/r/59620/diff/7-8/


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=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.

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

> On May 7, 2018, 12:14 p.m., Peter Cseh wrote:
> > examples/src/main/apps/git/workflow.xml
> > Lines 24-25 (patched)
> > 
> >
> > Let's check out apache/Oozie from github instead :)

Yes, I had an interim smallish repo. Unfortunately, JGit seems to have issues 
with branches and repo depth at this time -- two features that will really help 
to add in the Git action once it gets off the ground. A full checkout of Oozie 
over even a reasonable coffee shop wifi (~200 KiB/s) can be ~5+ minutes so we 
should use something small. Andras suggested Scoozi in his patches but I would 
prefer something in Apache. Any Apache suggestions for repos which might be 
smaller?


- Clay


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review202546
---


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=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.
> 
>

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

> On May 7, 2018, 12:13 p.m., Peter Cseh wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
> > Lines 55-56 (patched)
> > 
> >
> > oozie.oozie? seems redundant :)

Ah yes, I see now.


- Clay


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review202545
---


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=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.
> 
>

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

> On July 2, 2018, 1:04 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
> > Lines 121-127 (patched)
> > 
> >
> > Why are we setting/checking these low level properties? Eg. workflow ID 
> > is already set at this point. Callback URL ditto, it's handled by 
> > JavaActionExecutor.
> > 
> > The order of parameters passed to checkAndSet() is also confusing, it 
> > should be key, then value, not the other way around.

I needed to leave the namenode setting in and of course the Git-action specific 
ones. I have refactored the `check*` methods to be key, value instead of the 
otherway around.


- Clay


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review205637
---


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=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.
> 
>

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

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

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

> On April 30, 2018, 3:12 p.m., Peter Cseh wrote:
> > client/src/main/resources/git-action-0.1.xsd
> > Lines 26-30 (patched)
> > 
> >
> > Plese use the new oozie-common namespace to make launcher config 
> > possible and use resoure-manager and instead of job-tracker. Take a look at 
> > the spark-action-1.0.xsd for reference

Andras has completed this making it look like the Spark XSD. The common XSD is 
a really nice building block!


> On April 30, 2018, 3:12 p.m., Peter Cseh wrote:
> > client/src/main/resources/git-action-0.1.xsd
> > Lines 32 (patched)
> > 
> >
> > I don't know if it's possible to define uris outside 
> > oozie.service.HadoopAccessorService.nameNode.whitelist here.
> > 
> > Can you please add a test for that?

Thanks Peter for thinking through this. I do not know that it be necessary we 
allow URI's other than the HadoopAccessorService whitelisted nameservices? 
Andras had a request that I rename `cloneRepoToHdfs` to be less opinionated but 
I presumed it would still be a whitelisted HCFS of some sort. For a test here, 
are you thinking of me using a `file://` path for 
`testWhenRepoIsClonedThenGitIndexContentIsReadSuccessfully()` in 
`TestIntegrationGitActionExecutor.java` or can you provide me a bit more 
guidance or the use-case?


> On April 30, 2018, 3:12 p.m., Peter Cseh wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 56 (patched)
> > 
> >
> > I don't know if we shoudl provide compatibility to all the way back to 
> > mapred.job.tracker. I'd prefer to only look for 
> > yarn.resourcemanager.address.

I am sold; removed.


> On April 30, 2018, 3:12 p.m., Peter Cseh wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 64 (patched)
> > 
> >
> > This looks unneccessary. Where it's used?

Andras removed this dead code it appears.


> On April 30, 2018, 3:12 p.m., Peter Cseh wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 75-82 (patched)
> > 
> >
> > This is not used anywhere if I see it correctly

Andras removed this dead code too.


> On April 30, 2018, 3:12 p.m., Peter Cseh wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 137-145 (patched)
> > 
> >
> > I wonder how strict we should be with credential files. We might want 
> > to do a ssh-like check. Ssh fails if your .pem file is not readably only by 
> > your user, that's why we're setting the permissions here.
> > 
> > At least we could print out a warning to help users avoid leaking 
> > credentials to everyone from HDFS
> > 
> > Or we can go ssh-level strict and have an option to do fall back to 
> > warnings only.

Good idea! Indeed, I've had a hard time getting users to "do the right thing" 
in the past. As you see many more environments than I do, would you recommend 
we also check HDFS extended ACLs as well if available? Would you want an "allow 
insecure credential" boolean or would you want something more for the override?


> On April 30, 2018, 3:12 p.m., Peter Cseh wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 229-236 (patched)
> > 
> >
> > Please extract a method for creating local tmp folders.

Good idea. Also, I was not cleaning up the temporary directories it looks like.


- Clay


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review202124
---


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 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

> On June 5, 2018, 4:46 p.m., Robert Kanter wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
> > Lines 36 (patched)
> > 
> >
> > As Peter Bacsko pointed out on the JIRA, we shouldn't be fetching a 
> > remote repo during a unit test. Perhaps JGit has some easy way of mocking 
> > this or doing it locally?

Corrected to be a full unit-test spawning its own Git protocol server now.


- Clay


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review204323
---


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=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.
> 
>

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Peter Bacsko via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review205637
---




core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 121-127 (patched)


Why are we setting/checking these low level properties? Eg. workflow ID is 
already set at this point. Callback URL ditto, it's handled by 
JavaActionExecutor.

The order of parameters passed to checkAndSet() is also confusing, it 
should be key, then value, not the other way around.


- Peter Bacsko


On jún. 26, 2018, 9:50 du, Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated jún. 26, 2018, 9:50 du)
> 
> 
> 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=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.
> 
>

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Peter Bacsko via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review205636
---




core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 95 (patched)


Is this package private for a reason?



core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 226-227 (patched)


We shouldn't catch NPE let alone ignore it.



docs/src/site/twiki/WorkflowFunctionalSpec.twiki
Lines 1667-1669 (patched)


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.



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 41 (patched)


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.



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 166 (patched)


A better method name would be something like isValidUri()



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 185-188 (patched)


Is it necessary to validate these separately? If anything, these values 
should be checked inside Oozie, not when the container is alreay running.



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 201-204 (patched)


If we don't allow relative URIs (null scheme), then this check should be 
placed inside validUri().



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java
Lines 54-56 (patched)


Would be better to have these as final



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java
Lines 82-99 (patched)


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.



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
Lines 113 (patched)


Use Assert.fail() to indicate failure



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
Lines 116 (patched)


Assert.fail()



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMainGetKey.java
Lines 51-56 (patched)


Simplify this part, see related comment below



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
Lines 43 (patched)


As I mentioned above, let's avoid cloning a repository during test 
execution.



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
Lines 74-80 (patched)


This is way too complicated. Do we just read a contents of a file?

There are simpler means to do this: 
https://howtodoinjava.com/core-java/io/java-read-file-to-string-examples/



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
Lines 119-122 (patched)


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.


- Peter Bacsko


On jún. 26, 2018, 9:50 du, Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

---
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.


Changes
---

Upload of OOZIE-2877.013-1.patch


Bugs: OOZIE-2877
https://issues.apache.org/jira/browse/OOZIE-2877


Repository: oozie-git


Description
---

OOZIE-2877 - Oozie Git Action


Diffs (updated)
-

  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/

Changes: https://reviews.apache.org/r/59620/diff/6-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=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.

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

> On June 5, 2018, 4:46 p.m., Robert Kanter wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 216 (patched)
> > 
> >
> > Is this necessary?  Why not simply throw whatever exception 
> > (IOException, IllegalArgumentException, etc)?  We don't seem to be doing 
> > any special handling or anything for GitMainExceptions.

I was simply copying the idiom from JavaMain but do not have any broader plan 
for the exception; the same is true for `GitOperationsException`.


- Clay


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review204323
---


On June 4, 2018, 5:44 a.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated June 4, 2018, 5:44 a.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 c54db34 
>   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 6f35868 
>   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 67526d9 
> 
> 
> Diff: https://reviews.apache.org/r/59620/diff/6/
> 
> 
> 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=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.
> 
>

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Robert Kanter via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review204323
---




examples/src/main/apps/git/job.properties
Lines 9 (patched)


trailing whitespace; there's some other cases here and in other files too



sharelib/git/pom.xml
Lines 107-118 (patched)


This can be removed (It was needed some time ago in each sharelib pom file, 
but not anymore).



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 216 (patched)


Is this necessary?  Why not simply throw whatever exception (IOException, 
IllegalArgumentException, etc)?  We don't seem to be doing any special handling 
or anything for GitMainExceptions.



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
Lines 33 (patched)


"is" --> "in"



sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
Lines 36 (patched)


As Peter Bacsko pointed out on the JIRA, we shouldn't be fetching a remote 
repo during a unit test. Perhaps JGit has some easy way of mocking this or 
doing it locally?


- Robert Kanter


On June 4, 2018, 5:44 a.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated June 4, 2018, 5:44 a.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 c54db34 
>   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 6f35868 
>   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 67526d9 
> 
> 
> Diff: https://reviews.apache.org/r/59620/diff/6/
> 
> 
> 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=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.
> 
>

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/
---

(Updated June 4, 2018, 5:44 a.m.)


Review request for oozie and András Piros.


Changes
---

Latest set of changes addressing (nearly all) comments from three weeks ago


Bugs: OOZIE-2877
https://issues.apache.org/jira/browse/OOZIE-2877


Repository: oozie-git


Description
---

OOZIE-2877 - Oozie Git Action


Diffs (updated)
-

  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 c54db34 
  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 6f35868 
  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 67526d9 


Diff: https://reviews.apache.org/r/59620/diff/6/

Changes: https://reviews.apache.org/r/59620/diff/5-6/


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=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.

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Peter Cseh via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review202546
---




examples/src/main/apps/git/workflow.xml
Lines 24-25 (patched)


Let's check out apache/Oozie from github instead :)


- Peter Cseh


On May 4, 2018, 5:43 p.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated May 4, 2018, 5:43 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/java/org/apache/oozie/cli/OozieCLI.java 
> 4abc7502c0c9d8b59ded2baaed30c407ad073008 
>   client/src/main/resources/git-action-0.1.xsd PRE-CREATION 
>   core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
> PRE-CREATION 
>   core/src/main/resources/oozie-default.xml 
> 6f8925828090cee29a818de30a7c31db0617d816 
>   docs/src/site/twiki/WorkflowFunctionalSpec.twiki 
> 76cbe21ecd79eae86885a8db77831bf2c40f2d4f 
>   examples/src/main/apps/git/job.properties PRE-CREATION 
>   examples/src/main/apps/git/workflow.xml PRE-CREATION 
>   pom.xml d9fe1b20f571b1a6afacb3ce53f4d409dd3d60b1 
>   sharelib/git/pom.xml PRE-CREATION 
>   sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.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 6a0864d4e5b52160dc4e1a5cb9dfc34a037b67c8 
>   src/main/assemblies/sharelib.xml 07dc69c8276895b254e5af0cc021cce6ebad18f4 
>   webapp/pom.xml 797996912b6e6381b261a69f8eb1e012fe488fdf 
> 
> 
> Diff: https://reviews.apache.org/r/59620/diff/5/
> 
> 
> 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=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.
> 
>

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Peter Cseh via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review202545
---




core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 55-56 (patched)


oozie.oozie? seems redundant :)


- Peter Cseh


On May 4, 2018, 5:43 p.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated May 4, 2018, 5:43 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/java/org/apache/oozie/cli/OozieCLI.java 
> 4abc7502c0c9d8b59ded2baaed30c407ad073008 
>   client/src/main/resources/git-action-0.1.xsd PRE-CREATION 
>   core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
> PRE-CREATION 
>   core/src/main/resources/oozie-default.xml 
> 6f8925828090cee29a818de30a7c31db0617d816 
>   docs/src/site/twiki/WorkflowFunctionalSpec.twiki 
> 76cbe21ecd79eae86885a8db77831bf2c40f2d4f 
>   examples/src/main/apps/git/job.properties PRE-CREATION 
>   examples/src/main/apps/git/workflow.xml PRE-CREATION 
>   pom.xml d9fe1b20f571b1a6afacb3ce53f4d409dd3d60b1 
>   sharelib/git/pom.xml PRE-CREATION 
>   sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.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 6a0864d4e5b52160dc4e1a5cb9dfc34a037b67c8 
>   src/main/assemblies/sharelib.xml 07dc69c8276895b254e5af0cc021cce6ebad18f4 
>   webapp/pom.xml 797996912b6e6381b261a69f8eb1e012fe488fdf 
> 
> 
> Diff: https://reviews.apache.org/r/59620/diff/5/
> 
> 
> 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=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.
> 
>

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[András Piros via Review Board]

> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 84-88 (patched)
> > 
> >
> > Aren't there existing constants from Apache Hadoop project?
> 
> Clay B. wrote:
> I am not finding a clear authoritative sources in the 
> [https://github.com/apache/hadoop](Hadoop) project for `user.name` nor 
> `mapred.*.job.*`; would you have a suggestion? The 
> [JavaActionExecutor](https://github.com/apache/oozie/blob/83d4ddf45aa16649bd9fae367fa915379d5781cd/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java#L114-L146)
>  defines these all bespoke too sadly.
> 
> I did find YARN has the resource manager in 
> [https://github.com/apache/hadoop/blob/18c494a00c8ead768f3a868b450dceea485559df/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/conf/YarnConfiguration.java#L139-L148](YarnConfiguration.java).
> 
> Would it make more sense to have an Oozie constants class for these? (If 
> so, it looks like this might be a JIRA in its own scope with how often 
> repeated some of these strings are.)

`"user.name"` -> `Hive2Credentials.USER_NAME`, `OozieClient.USER_NAME`

For the MR1 config keys there isn't a current one in the Hadoop repo, for the 
newer ones:
`"mapreduce.jobtracker.address"` -> 
[`MRConfig.MASTER_ADDRESS`](https://github.com/apache/hadoop/blob/f67237cbe7bc48a1b9088e990800b37529f1db2a/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/MRConfig.java#L65)

I think less dependencies are always better, I'm for introducing these 
non-Oozie constants in `GitMain`, and reusing Oozie constants from code inside 
Oozie.


> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 199 (patched)
> > 
> >
> > Extract to another class.
> 
> Clay B. wrote:
> Please pardon my lack of creativity, however, are you thinking of a repo 
> class perhaps to encapsulate all the repository operations? Or a class for 
> some other reason?

I'm thinking of a `GitRepositoryOperations` class to encapsulate all the 
repository operation, having fields like `gitSrc`, `branch`, `outputDir`, 
`credentialFile` initialized in constructor.


> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 254 (patched)
> > 
> >
> > Extract to another class.
> 
> Clay B. wrote:
> Similarly, would this fit better in a repo class (if split off 
> independently) or independent of the repo code and `GitMain`?

I'm thinking of a `GitRepositoryOperations` class to encapsulate all the 
repository operation, having fields like `gitSrc`, `branch`, `outputDir`, 
`credentialFile` initialized in constructor.


> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
> > Lines 60 (patched)
> > 
> >
> > Are system props reset after this test run?
> 
> Clay B. wrote:
> I am not sure what the entire chain of `ActionExecutorTestCase`'s methods 
> are doing. However, I am not explicitly clearing this to my knowledge; do you 
> expect issues? (For reference, it seems 
> [`TestJavaActionExecutor()`](https://github.com/apache/oozie/blob/5998c18fde1da769e91e3ef1bcca484723730c76/core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java#L90)
>  is not clearing these either?)

I'd rather not set any system properties here but go for 
[`Services.get().getConf().set()`](https://github.com/apache/oozie/blob/eb168360c550943828d9fe2c7bfdcd4f5e830003/core/src/test/java/org/apache/oozie/service/TestActionService.java#L52-L53),
 if that works.

If not, I'd still set this system property in `@Before void setUp()` and reset 
in `@After void tearDown()` as this one is used by other important pieces of 
code.


> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
> > Lines 80 (patched)
> > 
> >
> > Please cut this into independent test cases, and name these accordingly.
> 
> Clay B. wrote:
> Could you provide me more direction on how you would ike this method 
> separated? Since the assertions in this method depend on the run state of the 
> action under test, I would need to run the action many times (all with the 
> same configuration); to me that would be a single case usually?

The current unit 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/
---

(Updated May 4, 2018, 5:43 p.m.)


Review request for oozie and András Piros.


Changes
---

Andras' Patch 011


Bugs: OOZIE-2877
https://issues.apache.org/jira/browse/OOZIE-2877


Repository: oozie-git


Description
---

OOZIE-2877 - Oozie Git Action


Diffs (updated)
-

  client/src/main/java/org/apache/oozie/cli/OozieCLI.java 
4abc7502c0c9d8b59ded2baaed30c407ad073008 
  client/src/main/resources/git-action-0.1.xsd PRE-CREATION 
  core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
PRE-CREATION 
  core/src/main/resources/oozie-default.xml 
6f8925828090cee29a818de30a7c31db0617d816 
  docs/src/site/twiki/WorkflowFunctionalSpec.twiki 
76cbe21ecd79eae86885a8db77831bf2c40f2d4f 
  examples/src/main/apps/git/job.properties PRE-CREATION 
  examples/src/main/apps/git/workflow.xml PRE-CREATION 
  pom.xml d9fe1b20f571b1a6afacb3ce53f4d409dd3d60b1 
  sharelib/git/pom.xml PRE-CREATION 
  sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.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 6a0864d4e5b52160dc4e1a5cb9dfc34a037b67c8 
  src/main/assemblies/sharelib.xml 07dc69c8276895b254e5af0cc021cce6ebad18f4 
  webapp/pom.xml 797996912b6e6381b261a69f8eb1e012fe488fdf 


Diff: https://reviews.apache.org/r/59620/diff/5/

Changes: https://reviews.apache.org/r/59620/diff/4-5/


Testing (updated)
---

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=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.

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/
---

(Updated May 4, 2018, 5:40 p.m.)


Review request for oozie and András Piros.


Changes
---

Andras' Patch 010


Bugs: OOZIE-2877
https://issues.apache.org/jira/browse/OOZIE-2877


Repository: oozie-git


Description
---

OOZIE-2877 - Oozie Git Action


Diffs (updated)
-

  client/src/main/java/org/apache/oozie/cli/OozieCLI.java 
4abc7502c0c9d8b59ded2baaed30c407ad073008 
  client/src/main/resources/git-action-0.1.xsd PRE-CREATION 
  core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
PRE-CREATION 
  core/src/main/resources/oozie-default.xml 
6f8925828090cee29a818de30a7c31db0617d816 
  docs/src/site/twiki/WorkflowFunctionalSpec.twiki 
76cbe21ecd79eae86885a8db77831bf2c40f2d4f 
  examples/src/main/apps/git/job.properties PRE-CREATION 
  examples/src/main/apps/git/workflow.xml PRE-CREATION 
  pom.xml d9fe1b20f571b1a6afacb3ce53f4d409dd3d60b1 
  sharelib/git/pom.xml PRE-CREATION 
  sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.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 6a0864d4e5b52160dc4e1a5cb9dfc34a037b67c8 
  src/main/assemblies/sharelib.xml 07dc69c8276895b254e5af0cc021cce6ebad18f4 
  webapp/pom.xml 797996912b6e6381b261a69f8eb1e012fe488fdf 


Diff: https://reviews.apache.org/r/59620/diff/4/

Changes: https://reviews.apache.org/r/59620/diff/3-4/


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


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.

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/
---

(Updated May 4, 2018, 5:38 p.m.)


Review request for oozie and András Piros.


Changes
---

My rebased patch from 20/Apr/18 06:38


Bugs: OOZIE-2877
https://issues.apache.org/jira/browse/OOZIE-2877


Repository: oozie-git


Description
---

OOZIE-2877 - Oozie Git Action


Diffs (updated)
-

  client/src/main/java/org/apache/oozie/cli/OozieCLI.java 4abc750 
  client/src/main/resources/git-action-0.1.xsd PRE-CREATION 
  core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
PRE-CREATION 
  core/src/main/resources/oozie-default.xml 6f89258 
  docs/src/site/twiki/WorkflowFunctionalSpec.twiki 5df6024 
  pom.xml d9fe1b2 
  sharelib/git/pom.xml PRE-CREATION 
  sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.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 7979969 


Diff: https://reviews.apache.org/r/59620/diff/3/

Changes: https://reviews.apache.org/r/59620/diff/2-3/


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


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.

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Peter Cseh via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review202124
---




client/src/main/java/org/apache/oozie/cli/OozieCLI.java
Lines 2168-2169 (patched)


Let's start with schema version 1.0



client/src/main/resources/git-action-0.1.xsd
Lines 26-30 (patched)


Plese use the new oozie-common namespace to make launcher config possible 
and use resoure-manager and instead of job-tracker. Take a look at the 
spark-action-1.0.xsd for reference



client/src/main/resources/git-action-0.1.xsd
Lines 32 (patched)


I don't know if it's possible to define uris outside 
oozie.service.HadoopAccessorService.nameNode.whitelist here.

Can you please add a test for that?



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 56 (patched)


I don't know if we shoudl provide compatibility to all the way back to 
mapred.job.tracker. I'd prefer to only look for yarn.resourcemanager.address.



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 64 (patched)


This looks unneccessary. Where it's used?



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 75-82 (patched)


This is not used anywhere if I see it correctly



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 137-145 (patched)


I wonder how strict we should be with credential files. We might want to do 
a ssh-like check. Ssh fails if your .pem file is not readably only by your 
user, that's why we're setting the permissions here.

At least we could print out a warning to help users avoid leaking 
credentials to everyone from HDFS

Or we can go ssh-level strict and have an option to do fall back to 
warnings only.



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 229-236 (patched)


Please extract a method for creating local tmp folders.


- Peter Cseh


On July 7, 2017, 1:27 a.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated July 7, 2017, 1:27 a.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/java/org/apache/oozie/cli/OozieCLI.java 4adf1a8 
>   client/src/main/resources/git-action-0.1.xsd PRE-CREATION 
>   core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
> PRE-CREATION 
>   core/src/main/resources/oozie-default.xml 5629a89 
>   docs/src/site/twiki/WorkflowFunctionalSpec.twiki 6bd3e5a 
>   pom.xml 16c5137 
>   sharelib/git/pom.xml PRE-CREATION 
>   sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.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 d794246 
>   src/main/assemblies/sharelib.xml 07dc69c 
>   webapp/pom.xml ee6341c 
> 
> 
> Diff: https://reviews.apache.org/r/59620/diff/2/
> 
> 
> 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).
> 
> 
> 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.
> 
>

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[András Piros via Review Board]

> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
> > Lines 53 (patched)
> > 
> >
> > `oozie.git.source.uri` would be better.
> 
> Clay B. wrote:
> Thanks! I for some reason thought there was a pattern 
> `oozie..` but I can not seem to find that anymore and your 
> suggestion makes things much less redundant.

Since the action here is `git` I think it's OK this way - and it even 
integrates w/ the existing pattern.


- András


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review176291
---


On July 7, 2017, 1:27 a.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated July 7, 2017, 1:27 a.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/java/org/apache/oozie/cli/OozieCLI.java 4adf1a8 
>   client/src/main/resources/git-action-0.1.xsd PRE-CREATION 
>   core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
> PRE-CREATION 
>   core/src/main/resources/oozie-default.xml 5629a89 
>   docs/src/site/twiki/WorkflowFunctionalSpec.twiki 6bd3e5a 
>   pom.xml 16c5137 
>   sharelib/git/pom.xml PRE-CREATION 
>   sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.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 d794246 
>   src/main/assemblies/sharelib.xml 07dc69c 
>   webapp/pom.xml ee6341c 
> 
> 
> Diff: https://reviews.apache.org/r/59620/diff/2/
> 
> 
> 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).
> 
> 
> 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.
> 
>

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/
---

(Updated July 7, 2017, 1:27 a.m.)


Review request for oozie and András Piros.


Changes
---

Review Board comments responded to so far. Still need Twiki documentation and 
breaks some `TestHDFSOperats` tests


Bugs: OOZIE-2877
https://issues.apache.org/jira/browse/OOZIE-2877


Repository: oozie-git


Description
---

OOZIE-2877 - Oozie Git Action


Diffs (updated)
-

  client/src/main/java/org/apache/oozie/cli/OozieCLI.java 4adf1a8 
  client/src/main/resources/git-action-0.1.xsd PRE-CREATION 
  core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
PRE-CREATION 
  core/src/main/resources/oozie-default.xml 5629a89 
  docs/src/site/twiki/WorkflowFunctionalSpec.twiki 6bd3e5a 
  pom.xml 16c5137 
  sharelib/git/pom.xml PRE-CREATION 
  sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.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 d794246 
  src/main/assemblies/sharelib.xml 07dc69c 
  webapp/pom.xml ee6341c 


Diff: https://reviews.apache.org/r/59620/diff/2/

Changes: https://reviews.apache.org/r/59620/diff/1-2/


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


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.

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

> On May 30, 2017, 11:08 a.m., András Piros wrote:
> >

I will keep cranking on the documentation but want to get your feedback on the 
changes so far; I've submitted the current code as a patch to the JIRA.


> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
> > Lines 53 (patched)
> > 
> >
> > `oozie.git.source.uri` would be better.

Thanks! I for some reason thought there was a pattern `oozie..` 
but I can not seem to find that anymore and your suggestion makes things much 
less redundant.


> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 82 (patched)
> > 
> >
> > Why not extend `JavaMain` instead?

I did not see an advantage over LauncherMain. Is there a reason to derrive from 
JavaMain?


> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 84-88 (patched)
> > 
> >
> > Aren't there existing constants from Apache Hadoop project?

I am not finding a clear authoritative sources in the 
[https://github.com/apache/hadoop](Hadoop) project for `user.name` nor 
`mapred.*.job.*`; would you have a suggestion? The 
[JavaActionExecutor](https://github.com/apache/oozie/blob/83d4ddf45aa16649bd9fae367fa915379d5781cd/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java#L114-L146)
 defines these all bespoke too sadly.

I did find YARN has the resource manager in 
[https://github.com/apache/hadoop/blob/18c494a00c8ead768f3a868b450dceea485559df/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/conf/YarnConfiguration.java#L139-L148](YarnConfiguration.java).

Would it make more sense to have an Oozie constants class for these? (If so, it 
looks like this might be a JIRA in its own scope with how often repeated some 
of these strings are.)


> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 103 (patched)
> > 
> >
> > `PROPERTIES_BLACKLIST` sounds better.

I'm happy to change this, but `DISALLOWED_PROPERTIES` came from the precident 
in 
[JavaActionExecutor](https://github.com/apache/oozie/blob/83d4ddf45aa16649bd9fae367fa915379d5781cd/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java#L147).


> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 144-145 (patched)
> > 
> >
> > Would be goot to `LOG.error()` before re-throwing. What about throwing 
> > a more action-specific `Exception` w/ the cause caught instead?

Cool; I see how JavaMain declares its own exceptions so I'll give that a try.


> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 154 (patched)
> > 
> >
> > Is it sure that the target is always HDFS? Can't it be a local path?

Good point! Any FileSystem can work; I have updated the code to sound less 
opinionated as to what FileSystem one is using.


> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 168 (patched)
> > 
> >
> > `copyKeyFromHdfs()` would be a better name.

Updated to `getKeyFromFS()`


> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 199 (patched)
> > 
> >
> > Extract to another class.

Please pardon my lack of creativity, however, are you thinking of a repo class 
perhaps to encapsulate all the repository operations? Or a class for some other 
reason?


> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 254 (patched)
> > 
> >
> > Extract to another class.

Similarly, would this fit better in a repo class (if split off independently) 
or independent of the repo code and `GitMain`?


> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java
> > Lines 125-128 (patched)
> > 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[András Piros]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review176291
---




core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 53 (patched)


`oozie.git.source.uri` would be better.



core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 54 (patched)


`oozie.git.branch` would be better.



core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 73 (patched)


What about throwing an `ActionExecutorException` instead?



core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 110-114 (patched)


Extract method.



core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 116-120 (patched)


Extract method.



core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 122-126 (patched)


Extract method.



core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 130-134 (patched)


Extract method.



core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 138-142 (patched)


Extract method.



core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 154-158 (patched)


Extract method.



core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 161-164 (patched)


Extract method.



core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 166-169 (patched)


Extract method.



docs/src/site/twiki/WorkflowFunctionalSpec.twiki
Lines 1617 (patched)


Please resolve this TODO.



docs/src/site/twiki/WorkflowFunctionalSpec.twiki
Lines 1621 (patched)


Please resolve this TODO.



docs/src/site/twiki/WorkflowFunctionalSpec.twiki
Lines 1624 (patched)


Please resolve this TODO.



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 82 (patched)


Why not extend `JavaMain` instead?



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 84-88 (patched)


Aren't there existing constants from Apache Hadoop project?



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 103 (patched)


`PROPERTIES_BLACKLIST` sounds better.



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 129 (patched)


Sounds like a good idea extracting `oozie.action.conf.xml` to a constant.



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 144-145 (patched)


Would be goot to `LOG.error()` before re-throwing. What about throwing a 
more action-specific `Exception` w/ the cause caught instead?



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 154 (patched)


Is it sure that the target is always HDFS? Can't it be a local path?



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 156-157 (patched)


Would be goot to `LOG.error()` before re-throwing. What about throwing a 
more action-specific `Exception` w/ the cause caught instead?



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 168 (patched)


`copyKeyFromHdfs()` would be a better name.



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 179-182 (patched)


Better to extract a `String` variable and reuse.



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 186-187 (patched)


Better to extract a `String` variable and reuse.



sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 199 (patched)

Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[Clay B.]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/
---

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/java/org/apache/oozie/cli/OozieCLI.java 38fb84e 
  client/src/main/resources/git-action-0.1.xsd PRE-CREATION 
  core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
PRE-CREATION 
  core/src/main/resources/oozie-default.xml 076401d 
  docs/src/site/twiki/WorkflowFunctionalSpec.twiki 6655696 
  pom.xml ebe1d68 
  sharelib/git/pom.xml PRE-CREATION 
  sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.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/TestGitMain.java 
PRE-CREATION 
  
sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
 PRE-CREATION 
  sharelib/pom.xml 190bd04 
  src/main/assemblies/sharelib.xml ea95c2e 
  webapp/pom.xml e4fdfb7 


Diff: https://reviews.apache.org/r/59620/diff/1/


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


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.