Refactor: convert DependenciesInfoTask to Java #41180
Conversation
dRadest
changed the title
|
Pinging @elastic/es-core-infra |
rhamedy
left a comment
rhamedy
left a comment
There was a problem hiding this comment.
Not part of the Elastic team but, left some comments anyway based on my learning experience from my PR for 39605 👍
| package org.elasticsearch.gradle; | ||
|
|
||
| import org.gradle.api.DefaultTask; | ||
| import org.elasticsearch.gradle.precommit.DependencyLicensesTask; |
There was a problem hiding this comment.
If I remember correctly, in this refactor effort you cannot reference groovy classes (i.e. DependencyLicensesTask) but, it's ok for groovy classes to reference this new java class. You might have to wait for #35231 to get merged first 🤔
There was a problem hiding this comment.
Yep, that was my general idea, i.e. to wait
There was a problem hiding this comment.
Sorry, I had some problems running the tests and stopped working on #35231. I'll try to see if I can make the tests work this weekend.
|
|
||
| private File outputFile = new File(getProject().getBuildDir(), "reports/dependencies/dependencies.csv"); | ||
|
|
||
| @Input |
There was a problem hiding this comment.
You do not need these annotations i.e. @Input anymore 👍 See review comments on other PRs.
There was a problem hiding this comment.
Thanks. I missed that. Is this just for the @input or all annotations?
| for(File file : matchedfiles) { | ||
| String prefix = file.getName().split("-LICENSE.*")[0]; | ||
| if (group.contains(prefix) || name.contains(prefix)) { | ||
| license = file.getAbsoluteFile(); |
There was a problem hiding this comment.
I know this is same as existing code but, makes me curious.
I wonder how this loop works? When the if (group.contains(prefix) || name.contains(prefix)) is true more than once then previous value of license is replaced with new one 🤔If it is true only once then we should return early after assigning license value. No? 😕
There was a problem hiding this comment.
I'm not sure, but I wonder if there could be more than one license file for a dependency? How would we know without looping through all of them? And if there are, looping through all of them and sticking to the last one makes as much sense as any other.
| if (license != null) { | ||
| // replace * because they are sometimes used at the beginning lines as if the license was a multi-line comment | ||
| final String content = new String(Files.readAllBytes(outfile.toPath()), StandardCharsets.UTF_8); | ||
| content.replaceAll("\\s+", " ").replaceAll("\\*", " "); |
There was a problem hiding this comment.
It kinda feels weird when invoking replaceAll on a final instance. Also replaceAll does not change the value of content and it returns the updated string and you are not storing it anywhere (hence changes lost). You might have to stick with existing solution
final String content = new String(license.readBytes(), "UTF-8").replaceAll("\\s+", " ").replaceAll("\\*", " ")
It does not change the value of a final field as well as the result of replaceAll on string is not lost 👍
There was a problem hiding this comment.
Ups! My bad. Thanks for pointing that out. But isn't readBytes() groovy ? I replaced it with Files.readAllBytes()
|
Thanks for picking this up @dRadest , and thanks for pitching in @rhamedy ! |
|
@elasticmachine update branch |
|
@elasticmachine test this please |
|
@dRadest looks like something is not wired correctly ? CI is picking up: |
|
I'm going to close this. @dRadest feel free to open it if you plan to work on it again |
7 participants
Contributes to #34459
status: ongoing
Done so far: