Skip to content

Fix dependency scopes#11574

Merged
hashhar merged 2 commits intotrinodb:masterfrom
wendigo:serafin/fix-dependency-scopes
Apr 4, 2022
Merged

Fix dependency scopes#11574
hashhar merged 2 commits intotrinodb:masterfrom
wendigo:serafin/fix-dependency-scopes

Conversation

@wendigo
Copy link
Copy Markdown
Contributor

@wendigo wendigo commented Mar 18, 2022

While testing maven-dependency-plugin update in airlift/airbase#304
it started reporting some dependencies as having invalid scope. These are now fixed and working both
on the old and new plugin versions.

@cla-bot cla-bot bot added the cla-signed label Mar 18, 2022
@wendigo wendigo requested review from ebyhr and findepi and removed request for findepi March 18, 2022 14:55
@wendigo wendigo force-pushed the serafin/fix-dependency-scopes branch 2 times, most recently from fea52ae to 30a81cc Compare March 18, 2022 15:14
@wendigo wendigo requested review from findepi and hashhar March 18, 2022 15:22
@findepi findepi requested a review from electrum March 18, 2022 16:46
@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 18, 2022

The scopes don't look correct to me, but it may be something i am missing.
in particular, runtime scope should rarely be needed, and it's used here quite a few times.

cc @electrum

@wendigo
Copy link
Copy Markdown
Contributor Author

wendigo commented Mar 18, 2022

@findepi then I don't know why all the tests have passed with this change 🤷‍♂️

Copy link
Copy Markdown
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

Overall looks good. Thanks for tracking these down.

@wendigo wendigo force-pushed the serafin/fix-dependency-scopes branch from 30a81cc to fb62973 Compare March 21, 2022 10:28
@wendigo wendigo requested review from electrum and findepi March 21, 2022 10:29
@findepi findepi removed their request for review March 21, 2022 10:55
@wendigo
Copy link
Copy Markdown
Contributor Author

wendigo commented Mar 21, 2022

@electrum ptal

@wendigo wendigo force-pushed the serafin/fix-dependency-scopes branch from fb62973 to a4efb58 Compare March 22, 2022 11:41
Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

I have doubts about correctness. Or maybe i just misunderstand how dependencies work. I've left a single comment as example of what I'm seeing wrong but I'm sure there are other such changes too.

@wendigo
Copy link
Copy Markdown
Contributor Author

wendigo commented Mar 25, 2022

@electrum can you take a look?

@wendigo wendigo requested review from findepi and hashhar March 29, 2022 12:17
@findepi findepi removed their request for review March 30, 2022 08:52
@wendigo wendigo force-pushed the serafin/fix-dependency-scopes branch from a4efb58 to c206d55 Compare April 1, 2022 14:32
@wendigo
Copy link
Copy Markdown
Contributor Author

wendigo commented Apr 1, 2022

@hashhar rebased to resolve conflicts, PTAL

wendigo added 2 commits April 1, 2022 20:11
While testing maven-dependency-plugin update in airlift/airbase#304
it started reporting some dependencies as having invalid scope. These are now fixed and working both
on the old and new plugin versions.
This version includes updated maven-dependency-plugin
@wendigo wendigo force-pushed the serafin/fix-dependency-scopes branch from c206d55 to f93de70 Compare April 1, 2022 18:11
@hashhar hashhar merged commit 03e4823 into trinodb:master Apr 4, 2022
@github-actions github-actions bot added this to the 376 milestone Apr 4, 2022
@hashhar hashhar added maintenance Project maintenance task no-release-notes This pull request does not require release notes entry labels Apr 4, 2022
@wendigo wendigo deleted the serafin/fix-dependency-scopes branch January 21, 2025 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed maintenance Project maintenance task no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

4 participants