Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TOQo1n9M] Core part - apoc-hadoop dependency is conflicting 5.x (neo4j-contrib/neo4j-apoc-procedures#3450) #347

Merged
merged 2 commits into from
Apr 3, 2023

Conversation

vga91
Copy link
Collaborator

@vga91 vga91 commented Mar 13, 2023

Cherry-pick (core part) of neo4j-contrib/neo4j-apoc-procedures#3450

Extended part: neo4j-contrib/neo4j-apoc-procedures#3454.

  • Because of split, now we have 2 equivalent apoc.ApocSignatures which collect the list of procedures and functions,
    one for core and one for extended, so I added a String projectPath in order to create 2 different classes (ApocSignaturesCore and ApocSignaturesExtended).
  • Created a copyFilesToPlugin(..) to be reused in Extended
  • Changed baseDir, pluginsFolder and extendedDir to public, reusable in Ext. too

@vga91 vga91 marked this pull request as draft March 13, 2023 16:34
@vga91 vga91 force-pushed the hadoop-dependency-conflict_core-dev branch from 569971c to 6c20011 Compare March 14, 2023 11:00
@vga91 vga91 force-pushed the hadoop-dependency-conflict_core-dev branch from 6c20011 to 8d4740b Compare March 14, 2023 13:35
@vga91 vga91 changed the title [TOQo1n9M] [WIP] Core part - apoc-hadoop dependency is conflicting 5.x (neo4j-contrib/neo4j-apoc-procedures#3450) [TOQo1n9M] Core part - apoc-hadoop dependency is conflicting 5.x (neo4j-contrib/neo4j-apoc-procedures#3450) Mar 14, 2023
@vga91 vga91 marked this pull request as ready for review March 14, 2023 16:48
@vga91
Copy link
Collaborator Author

vga91 commented Mar 14, 2023

@Lojjs I know it's a cherry pick of a pr you already approved,
but due to the split in 5.x I had to make a few changes from the original one.

Do you (or someone else) want to take a look here as well, by any chance?

@Lojjs Lojjs self-assigned this Mar 15, 2023
Copy link
Contributor

@Lojjs Lojjs left a comment

Choose a reason for hiding this comment

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

I have a couple of concerns, not sure if my ideas work though

Comment on lines 44 to 47
// create and delete a file to retrieve the current project (`core` or `extended`)
FileObject resource = filer.createResource(StandardLocation.SOURCE_OUTPUT, "", "tmp", (Element[]) null);
String projectPath = resource.getName();
resource.delete();
Copy link
Contributor

Choose a reason for hiding this comment

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

This feel a bit over complicated to me. Cannot we not e.g. send in a boolean to the write method depending on if we call it from core or extended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed with a boolean

Copy link
Contributor

Choose a reason for hiding this comment

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

I was more thinking about if we could avoid creating a temporary file, but looking closer I think it might not be so easy to do it in a smarter way. With your change it is at least a bit cleaner :)

@@ -75,8 +75,8 @@ public void compare_with_sources() {
.list(record -> record.get("name").asString());


assertEquals(sorted(ApocSignatures.PROCEDURES), procedureNames);
assertEquals(sorted(ApocSignatures.FUNCTIONS), functionNames);
assertEquals(sorted(ApocSignaturesCore.PROCEDURES), procedureNames);
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a brief look at your extended PR too. How come we test both core and extended in extended PR? I would (maybe naively) have thought that it would make more sense to have the checkCoreWithExtraDependenciesJars test here and the other 2 tests in the extended repo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could be wrong, but I don't think it's possible to test core and extra-dependencies here,
because we have to build the latter directly in extended,
I think it's not possible to build them from core.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok. I don't think I actually have tried to build them myself, so you might be right about that. If it was a conscious decision, I think it is ok.

Copy link
Contributor

@Lojjs Lojjs left a comment

Choose a reason for hiding this comment

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

With your new changes, I think this is good enough

@vga91 vga91 merged commit 3ab40ae into dev Apr 3, 2023
@vga91 vga91 deleted the hadoop-dependency-conflict_core-dev branch April 3, 2023 15:21
vga91 added a commit that referenced this pull request Apr 12, 2023
…4j-contrib/neo4j-apoc-procedures#3450) (#347)

* [TOQo1n9M] Core part - apoc-hadoop dependency is conflicting 5.x (neo4j-contrib/neo4j-apoc-procedures#3450)

* [TOQo1n9M] changed getProjectPath() to isExtendedProject()
@vga91 vga91 added the dev label Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants