Converting DependencyLicensesTask and UpdateShasTask to java #41921
Conversation
Megamiun
force-pushed
the
refactor/updateshastask-to-java
branch
from
6057570 to
93cc4bd
Compare
|
Pinging @elastic/es-core-infra |
|
@elasticmachine test this please |
|
Hello, I see it failed on |
|
that looks unrelated @elasticmachine run elasticsearch-ci/1 |
|
@atorok Do you know what I can do to correct this test? Should we ask the checks to rerun? |
|
@Megamiun This one doesn't seem related either, but a rerun won't work for sure without merging master in. |
Megamiun
force-pushed
the
refactor/updateshastask-to-java
branch
from
fc71b2b to
a9462d9
Compare
|
@atorok I rebased it. If needed when you will run the pipeline, I think you can merge/rebase it again. (Not sure, but I think that yes) |
|
@elasticmachine test this please. |
|
@atorok Ah, ok, I will remember to merge instead of rebase in the future |
|
Hello, @atorok, just to know, when will this PR be merged? I think there is another PR waiting for this one to be approved to be finished. |
| Set<File> getShaFiles() { | ||
| File[] array = licensesDir.listFiles(); | ||
| if (array == null) { | ||
| return Collections.emptySet(); |
There was a problem hiding this comment.
licensesDir should always be a directory, so we should throw an exception if we get a null here .
| } | ||
|
|
||
| String getSha1(File file) throws IOException, NoSuchAlgorithmException { | ||
| return applySha1(getBytes(file)); |
There was a problem hiding this comment.
I would inline applySha1 as otherwise getSha1 is a lazy method., same with getBytes just makes things harder to read.
| import org.elasticsearch.gradle.test.GradleIntegrationTestCase; | ||
| import org.gradle.testkit.runner.GradleRunner; | ||
|
|
||
| public class DependencyLicensesTasksIT extends GradleIntegrationTestCase { |
There was a problem hiding this comment.
Do you think there's any added benefit of the integration test over the unit test ?
The only one I see is that it checks that the task is wired correctly, but it does a lot more checks that seem redundant. The unit test is probably sufficient in this case. A unit test for PrecommitTasks could check the wiring if we wanted to ( not saying it needs to go in this PR ).
|
Sorry for the lateness, made the changes. But I have one doubt, should really DependenciesInfoTask receive a LinkedHashMap and DependencyLicensesTask too? Wouldn't it be better that it just returned a common Map? |
|
@elasticmachine test this please |
|
@elasticmachine update branch |
|
@elasticmachine test this please |
Relates to #34459 and #35231.
Unfortunately, I closed the previous PR on my changes while cleaning my repo.
For more info, see #35231