Skip to content

Restore running Maven checks by default#11309

Merged
findepi merged 5 commits intotrinodb:masterfrom
nineinchnick:fix-maven-checks-violations
Mar 5, 2022
Merged

Restore running Maven checks by default#11309
findepi merged 5 commits intotrinodb:masterfrom
nineinchnick:fix-maven-checks-violations

Conversation

@nineinchnick
Copy link
Copy Markdown
Member

@nineinchnick nineinchnick commented Mar 3, 2022

Description

Restore running Maven checks by default.

Since it was disabled for two days, also fix:

  • dependency issues after adding trino-exchange to trino-tests;
  • imports order.

Is this change a fix, improvement, new feature, refactoring, or other?
other

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)
tests

How would you describe this change to a non-technical end user or system administrator?
n/a

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@nineinchnick nineinchnick requested review from findepi and losipiuk March 3, 2022 16:04
losipiuk
losipiuk reviewed Mar 3, 2022
View reviewed changes
nineinchnick
nineinchnick commented Mar 3, 2022
View reviewed changes
losipiuk
losipiuk approved these changes Mar 3, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

LGTM.
Yet as discussed offline it may be better to add trino-exchange as a provided dependency of trino-testing. And add it for real only to modules which actually make use of the classes from there (trino-tests and maybe sth else).

I did not verify if this approach would work - but seems as it should.

@hashhar , @findepi opinions?

@github-actions github-actions bot added jdbc Relates to Trino JDBC driver tests:hive labels Mar 3, 2022
findepi
findepi reviewed Mar 3, 2022
View reviewed changes
findepi
findepi reviewed Mar 3, 2022
View reviewed changes
@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 3, 2022

Yet as discussed offline it may be better to add trino-exchange as a provided dependency of trino-testing. And add it for real only to modules which actually make use of the classes from there (trino-tests and maybe sth else).

I would suggest removing it from there.
I.e redo (or revert) a0d8caa cc @losipiuk

@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 3, 2022

Let me try: #11311

@findepi findepi mentioned this pull request Mar 3, 2022
Closed
@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 3, 2022

Can you please rebase on top of #11311 ?

@nineinchnick nineinchnick force-pushed the fix-maven-checks-violations branch from c225481 to d02dbeb Compare March 3, 2022 17:57
@cla-bot cla-bot bot added the cla-signed label Mar 3, 2022
@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 3, 2022

Can you please rebase on top of #11311 ?

Can you please do this again? #11311 was broken.

@nineinchnick nineinchnick force-pushed the fix-maven-checks-violations branch from d02dbeb to c60280b Compare March 4, 2022 07:38
findepi
findepi approved these changes Mar 4, 2022
View reviewed changes
@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 4, 2022

@nineinchnick see red CI

@nineinchnick nineinchnick force-pushed the fix-maven-checks-violations branch from c60280b to d39cd1a Compare March 4, 2022 10:05
@nineinchnick
Copy link
Copy Markdown
Member Author

nineinchnick commented Mar 4, 2022

@nineinchnick see red CI

I swear I ran a build locally to check it first ... Rebased on the latest master and I checked locally again.

@nineinchnick nineinchnick force-pushed the fix-maven-checks-violations branch 2 times, most recently from b66c7ab to 4dea871 Compare March 4, 2022 16:24
findepi added 2 commits March 4, 2022 20:23
@findepi
Remove trino-testing dependency on trino-exchange
7a18ebf 
`trino-exchange` has lots of dependencies, and `trino-testing` is used
in all modules. This causes classpath conflicts which were not detected
because maven dependency checker was not being run (fixed in airbase
122).

This commit adds a bit of code duplication, but reduces dependencies.
@findepi
Use current name of the configuration property
692d86a 
Was renamed in f85eaab.
@nineinchnick nineinchnick force-pushed the fix-maven-checks-violations branch from 4dea871 to 1558a7f Compare March 4, 2022 19:35
Jan Was added 2 commits March 4, 2022 23:36
@findepi
Fix imports order
92c5c7f
@findepi
Update Airbase to restore running Maven checks by default
0961b0a 
Airbase since 120 does not enable plugins marked as skipped using
`air.check.skip-*` properties, but it wasn't properly handling property
expansion. Airbase 122 does not enable check plugins only when
`air.check.skip-all` is set, and skips them using configuration in other
cases.
@findepi findepi force-pushed the fix-maven-checks-violations branch from 1558a7f to 0961b0a Compare March 4, 2022 22:36
@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 4, 2022

added fixes for new imports ordering violations from #11312

@findepi findepi merged commit ee5c3f2 into trinodb:master Mar 5, 2022
@github-actions github-actions bot added this to the 373 milestone Mar 5, 2022
@nineinchnick nineinchnick deleted the fix-maven-checks-violations branch March 5, 2022 09:46
@mosabua mosabua mentioned this pull request Mar 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@findepi findepi findepi approved these changes

@losipiuk losipiuk losipiuk approved these changes

Assignees

No one assigned

Labels

cla-signed jdbc Relates to Trino JDBC driver

Milestone

373

Development

Successfully merging this pull request may close these issues.

3 participants

@nineinchnick @findepi @losipiuk