Skip to content

Enable compiler warnings in xpack core plugin project#66899

Merged
pugnascotia merged 18 commits intoelastic:masterfrom
vamuzumd:vamuzumd/LowHangingFirst
Mar 22, 2021
Merged

Enable compiler warnings in xpack core plugin project#66899
pugnascotia merged 18 commits intoelastic:masterfrom
vamuzumd:vamuzumd/LowHangingFirst

Conversation

@vamuzumd
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

elasticmachine commented Jan 4, 2021

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

@vamuzumd
Copy link
Copy Markdown
Contributor Author

vamuzumd commented Jan 24, 2021

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
Copy Markdown
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
Copy Markdown
Contributor Author

vamuzumd commented Jan 27, 2021

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
Copy Markdown
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
Copy Markdown
Contributor

pugnascotia commented Jan 27, 2021

@elasticmachine test this please

@pugnascotia
Copy link
Copy Markdown
Contributor

pugnascotia commented Jan 27, 2021

@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
private ArgumentCaptor<CreateIndexRequest> createRequestCaptor;
private ArgumentCaptor<IndicesAliasesRequest> aliasesRequestCaptor;

@SuppressWarnings("unchecked")
Copy link
Copy Markdown
Contributor

@pugnascotia pugnascotia Mar 11, 2021

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
Copy Markdown
Contributor Author

@vamuzumd vamuzumd Mar 11, 2021

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
Copy Markdown
Contributor

pugnascotia commented Mar 11, 2021

@elasticmachine ok to test

@pugnascotia
Copy link
Copy Markdown
Contributor

pugnascotia commented Mar 11, 2021

@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
Copy Markdown
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
Copy Markdown
Contributor Author

vamuzumd commented Mar 15, 2021

@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
Copy Markdown
Contributor

pugnascotia commented Mar 22, 2021 •
edited
Loading

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

@pugnascotia
Copy link
Copy Markdown
Contributor

pugnascotia commented Mar 22, 2021

@elasticmachine update branch

pugnascotia
pugnascotia approved these changes Mar 22, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

pugnascotia commented Mar 22, 2021

Thank you for your work @vamuzumd!

@pugnascotia
Copy link
Copy Markdown
Contributor

pugnascotia commented Mar 22, 2021

@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 <elasticmachine@users.noreply.github.com>
@pugnascotia
Copy link
Copy Markdown
Contributor

pugnascotia commented Mar 22, 2021

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