Skip to content

converting ForbiddenPatternsTask to .java #36194

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Dec 11, 2018
Merged

converting ForbiddenPatternsTask to .java #36194

merged 15 commits into from
Dec 11, 2018

Conversation

cdschneider
Copy link
Contributor

a part of #34459 specifically ForbiddenPatternsTask

@alpar-t alpar-t added :Delivery/Build Build or test infrastructure v7.0.0 v6.6.0 labels Dec 4, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

alpar-t
alpar-t reviewed Dec 4, 2018
View reviewed changes
Copy link
Contributor

@alpar-t alpar-t left a comment

Choose a reason for hiding this comment

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

Thanks for you contribution @cdschneider !
I left a few minor comments.
Some of them were already there in the Groovy code but I think this is a good opportunity to take care of them.

@alpar-t
Copy link
Contributor

alpar-t commented Dec 4, 2018

@elasticmachine test this please

@alpar-t
Copy link
Contributor

alpar-t commented Dec 4, 2018

@elasticmachine test this please

alpar-t
alpar-t reviewed Dec 6, 2018
View reviewed changes
Copy link
Contributor

@alpar-t alpar-t left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @cdschneider ! We're almost there, I left just a few comments for small things.

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java
.stream()
.map(sourceSet -> sourceSet.getAllSource().matching(filesFilter))
.reduce(FileTree::plus)
.orElse(getProject().files().getAsFileTree());
Copy link
Contributor

Choose a reason for hiding this comment

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

getProject().files() returns a file collection, no need for getAsFileTree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the result of reduce() is an Optional<FileTree>, doesn't the result of the .orElse() statement also need be of type FileTree?

@alpar-t
Copy link
Contributor

alpar-t commented Dec 6, 2018

There are non UTF-8 files in other projects too it seems.

* What went wrong:
Execution failed for task ':x-pack:qa:full-cluster-restart:forbiddenPatterns'.
> java.nio.charset.MalformedInputException: Input length = 1

Might be worth to catch the exception and wrap it to include the offending file in the error message. You'll be able to fond them all by running ./gradlew precommit

@alpar-t
Copy link
Contributor

alpar-t commented Dec 7, 2018

@elasticmachine test this please

@alpar-t alpar-t mentioned this pull request Dec 10, 2018
Open
31 tasks
@alpar-t
Copy link
Contributor

alpar-t commented Dec 10, 2018

@elasticmachine test this please

@alpar-t
Copy link
Contributor

alpar-t commented Dec 11, 2018

Thanks again for your contribution @cdschneider, we really appreciate it!

@alpar-t alpar-t merged commit 00eadd9 into elastic:master Dec 11, 2018
alpar-t pushed a commit that referenced this pull request Dec 11, 2018
@cdschneider @alpar-t
converting ForbiddenPatternsTask to .java (#36194)
e5940ee 
* converting ForbiddenPatternsTask to java impl & unit tests
@cdschneider
Copy link
Contributor Author

No problem! Glad I could help! 😄 I will definitely be trying to contributie again soon!

jasontedor added a commit to dnhatn/elasticsearch that referenced this pull request Dec 11, 2018
@jasontedor
Merge remote-tracking branch 'elastic/master' into pr/36141
7fc6dc2 
* elastic/master: (36 commits)
  Add check for minimum required Docker version (elastic#36497)
  Minor search controller changes (elastic#36479)
  Add default methods to DocValueFormat (elastic#36480)
  Fix the mixed cluster REST test explain/11_basic_with_types.
  Modify `BigArrays` to take name of circuit breaker (elastic#36461)
  Move LoggedExec to minimumRuntime source set (elastic#36453)
  added 6.5.4 version
  Add test logging for elastic#35644
  Tests- added helper methods to ESRestTestCase for checking warnings (elastic#36443)
  SQL: move requests' parameters to requests JSON body (elastic#36149)
  [Zen2] Respect the no_master_block setting (elastic#36478)
  Require soft-deletes when access changes snapshot (elastic#36446)
  Switch more tests to zen2 (elastic#36367)
  [Painless] Add extensive tests for def to primitive casts (elastic#36455)
  Add setting to bypass Rollover action (elastic#36235)
  Try running CI against Zulu (elastic#36391)
  [DOCS] Reworked the shard allocation filtering info.  (elastic#36456)
  Log [initial_master_nodes] on formation failure (elastic#36466)
  converting ForbiddenPatternsTask to .java (elastic#36194)
  fixed typo
  ...
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alpar-t alpar-t alpar-t left review comments

Assignees
No one assigned
Labels
:Delivery/Build Build or test infrastructure >non-issue Team:Delivery Meta label for Delivery team v6.6.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cdschneider @elasticmachine @alpar-t @colings86 @mark-vieira