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.

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

Enable compiler warnings in xpack core plugin project #66899

Merged
merged 18 commits into from
Mar 22, 2021
Merged

Enable compiler warnings in xpack core plugin project #66899

merged 18 commits into from
Mar 22, 2021

Conversation

vamuzumd
Copy link
Contributor

@vamuzumd vamuzumd commented Dec 31, 2020 •
edited
Loading

This commit is part of issue #40366 to remove disabled lint warnings from gradle files. This PR renables Xlint warnings for XPack/plugin/core project by:-

  • Adding wildcard type parameters
  • Suppressing unchecked warnings for most occurrences as they involved converting Object into Collection(List, Map) within lambda function.
  • Removed dead url -// https://github.com/elastic/x-plugins/issues/724
  • Suppressing rawtype warnings for ExecutableChainTransform class as it returns derived class object having more than 1 derived types.
  • Adding unchecked suppressions for randomFrom method usages as it returns a generic object.

@vamuzumd vamuzumd mentioned this pull request Dec 31, 2020
Closed
23 tasks
@vamuzumd vamuzumd changed the title Enabled Lint warnings in XPack-Plugin-Core project Enabled XLint warnings in XPack-Plugin-Core project Dec 31, 2020
@vamuzumd vamuzumd force-pushed the vamuzumd/LowHangingFirst branch from 364943e to e369db6 Compare December 31, 2020 22:45
@matriv matriv added the :Delivery/Build Build or test infrastructure label Jan 4, 2021
@elasticmachine elasticmachine added the Team:Delivery Meta label for Delivery team label Jan 4, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@vamuzumd
Copy link
Contributor Author

Hi @martijnvg , @pugnascotia , could you please review this PR?

@pugnascotia pugnascotia self-requested a review January 24, 2021 12:09
pugnascotia
pugnascotia requested changes Jan 25, 2021
View reviewed changes
Copy link
Contributor

@pugnascotia pugnascotia left a comment

Choose a reason for hiding this comment

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

Thank you very much @vamuzumd, the PR needs a couple of tweaks but otherwise looks good.

@vamuzumd vamuzumd force-pushed the vamuzumd/LowHangingFirst branch from 5c775fd to c420a27 Compare January 27, 2021 02:42
@vamuzumd vamuzumd force-pushed the vamuzumd/LowHangingFirst branch from c6bcaa9 to e32d7f1 Compare January 27, 2021 02:58
@vamuzumd vamuzumd requested a review from pugnascotia January 27, 2021 03:01
@vamuzumd
Copy link
Contributor Author

Thank you very much @vamuzumd, the PR needs a couple of tweaks but otherwise looks good.

Thanks for the feedback @pugnascotia !

@vamuzumd vamuzumd closed this Jan 27, 2021
@vamuzumd vamuzumd reopened this Jan 27, 2021
@breskeby breskeby self-requested a review January 27, 2021 08:22
breskeby
breskeby approved these changes Jan 27, 2021
View reviewed changes
Copy link
Contributor

@breskeby breskeby left a comment

Choose a reason for hiding this comment

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

lgtm apart from what @pugnascotia already mentioned

@pugnascotia
Copy link
Contributor

@elasticmachine test this please

@pugnascotia
Copy link
Contributor

@vamuzumd there are compilation failures. I suggest running ./gradlew precommit to make sure the code all compiles and passes our style checks.

pugnascotia
pugnascotia reviewed Mar 11, 2021
View reviewed changes
...ck/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/utils/MlIndexAndAliasTests.java
@@ -81,6 +81,7 @@
private ArgumentCaptor<CreateIndexRequest> createRequestCaptor;
private ArgumentCaptor<IndicesAliasesRequest> aliasesRequestCaptor;

@SuppressWarnings("unchecked")
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use the anyActionListener() trick here again, as well as defining a mockActionListener() for the other compiler error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, we could do that, but like the other case, this requires defining 4 helpers plus address the casting on line 100, hence leaving this as is. But please lmk if you think otherwise.

@pugnascotia
Copy link
Contributor

@elasticmachine ok to test

@pugnascotia
Copy link
Contributor

@vamuzumd this is looking good, just a few things to fix up. I appreciate you tackling this work, some of the generics are...interesting 😱

@vamuzumd vamuzumd force-pushed the vamuzumd/LowHangingFirst branch from 99c0547 to ac39aae Compare March 11, 2021 23:49
@vamuzumd
Copy link
Contributor Author

vamuzumd commented Mar 12, 2021 •
edited
Loading

Seems like the test BootStrapTests.testTriggeredWatchLoading failed for elasticsearch-ci/2, which seems flaky and unrelated to my change based on stack trace. @pugnascotia, thoughts?

Partial stacktrace

java.lang.AssertionError: Count is 18 hits but 9 was expected. Total shards: 1 Successful shards: 1 & 0 shard failures:
at __randomizedtesting.SeedInfo.seed([76DF43B200299F27:4F3123083FC3A7D8]:0)
at org.junit.Assert.fail(Assert.java:88)
at org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount(ElasticsearchAssertions.java:259)
at org.elasticsearch.xpack.watcher.test.integration.BootStrapTests.lambda$assertSingleExecutionAndCompleteWatchHistory$3(BootStrapTests.java:289)

@vamuzumd vamuzumd requested a review from pugnascotia March 12, 2021 09:09
@vamuzumd
Copy link
Contributor Author

@pugnascotia, could you please review/sign-off this PR? Just hoping to avoid another merge conflict since the PR has a large number of files changed.

@pugnascotia
Copy link
Contributor

pugnascotia commented Mar 22, 2021 •
edited
Loading

Sorry @vamuzumd, I've been on vacation for the past week.

@pugnascotia
Copy link
Contributor

@elasticmachine update branch

pugnascotia
pugnascotia approved these changes Mar 22, 2021
View reviewed changes
Copy link
Contributor

@pugnascotia pugnascotia left a comment

Choose a reason for hiding this comment

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

LGTM!

@pugnascotia pugnascotia changed the title Enabled XLint warnings in XPack-Plugin-Core project Enable compiler warnings in xpack core plugin project Mar 22, 2021
@pugnascotia pugnascotia merged commit 6cc0670 into elastic:master Mar 22, 2021
@pugnascotia
Copy link
Contributor

Thank you for your work @vamuzumd!

@pugnascotia
Copy link
Contributor

@vamuzumd sorry for the noise, I should have added the right labels sooner.

pugnascotia pushed a commit that referenced this pull request Mar 22, 2021
@vamuzumd @elasticmachine @pugnascotia
Enable compiler warnings in xpack core plugin project (#66899)
37a24d5 
Co-authored-by: Elastic Machine <[email protected]>
@pugnascotia
Copy link
Contributor

Backported to 7.x in 37a24d5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breskeby breskeby breskeby approved these changes

@pugnascotia pugnascotia pugnascotia approved these changes

Assignees

@pugnascotia pugnascotia

Labels
:Delivery/Build Build or test infrastructure >refactoring Team:Delivery Meta label for Delivery team v7.13.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vamuzumd @elasticmachine @pugnascotia @breskeby @jakelandis @matriv