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

fix: #2470 Use mirror to download dependencies if applicable #2617

Merged
merged 1 commit into from
May 29, 2019
Merged

fix: #2470 Use mirror to download dependencies if applicable #2617

merged 1 commit into from
May 29, 2019

Conversation

jycr
Copy link
Contributor

@jycr jycr commented May 28, 2019 •
edited
Loading

fix: #2470 Use mirror to download dependencies if applicable.
And ensure that proxy config is setup if needed

This was referenced May 28, 2019
Closed
Closed
Closed
@mkouba mkouba requested review from gsmet and aloubyansky May 28, 2019 09:20
@aloubyansky
Copy link
Member

@jycr wouldn't this change aloubyansky@e5a1bac be enough?

@jycr
Copy link
Contributor Author

jycr commented May 28, 2019 •
edited
Loading

@jycr wouldn't this change aloubyansky@e5a1bac be enough?

I think there still little problems with your proposal:

  • if a mirror is applicable for several RemoteRepository, we will have several same RemoteRepository definition in return result array
  • if a mirror need to be connected through proxy, it doesn't work because proxy setup, according mirror target, is not applied

@jycr
Copy link
Contributor Author

jycr commented May 28, 2019

I hope It is not too late for this PR for 0.16.0 release ?

aloubyansky
aloubyansky requested changes May 28, 2019
View reviewed changes
Copy link
Member

@aloubyansky aloubyansky left a comment

Choose a reason for hiding this comment

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

No, it's not too late. I've added a few remarks, could you please address those? Please avoid lambdas when they are not necessary. Thanks!

...otstrap/core/src/main/java/io/quarkus/bootstrap/resolver/maven/ProxyAwareMirrorSelector.java Outdated Show resolved Hide resolved
...otstrap/core/src/main/java/io/quarkus/bootstrap/resolver/maven/ProxyAwareMirrorSelector.java Outdated Show resolved Hide resolved
...otstrap/core/src/main/java/io/quarkus/bootstrap/resolver/maven/ProxyAwareMirrorSelector.java Outdated Show resolved Hide resolved
...s/bootstrap/core/src/main/java/io/quarkus/bootstrap/resolver/maven/MavenRepoInitializer.java Outdated Show resolved Hide resolved
@jycr
Copy link
Contributor Author

jycr commented May 28, 2019 •
edited
Loading

I have simplified implementation and move proxy setup (and NULL_PROXY_SELECTOR) into ProxyAwareMirrorSelector (see: jycr@a7c7d079801d935439e540d0ad06d2a2dd83e50a)

aloubyansky
aloubyansky requested changes May 29, 2019
View reviewed changes
Copy link
Member

@aloubyansky aloubyansky left a comment

Choose a reason for hiding this comment

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

A few minor remarks. Thanks a lot, @jycr !

...s/bootstrap/core/src/main/java/io/quarkus/bootstrap/resolver/maven/MavenRepoInitializer.java Outdated Show resolved Hide resolved
...otstrap/core/src/test/java/io/quarkus/bootstrap/resolver/maven/MavenRepoInitializerTest.java Outdated Show resolved Hide resolved
...otstrap/core/src/test/java/io/quarkus/bootstrap/resolver/maven/MavenRepoInitializerTest.java Outdated Show resolved Hide resolved
...otstrap/core/src/test/java/io/quarkus/bootstrap/resolver/maven/MavenRepoInitializerTest.java Outdated Show resolved Hide resolved
...otstrap/core/src/test/java/io/quarkus/bootstrap/resolver/maven/MavenRepoInitializerTest.java Outdated Show resolved Hide resolved
@gsmet
Copy link
Member

gsmet commented May 29, 2019

@aloubyansky I let you drive this one home before the release? Thanks!

@gsmet gsmet added this to the 0.16.0 milestone May 29, 2019
@aloubyansky
Copy link
Member

Ok, given that we don't have time, let's merge it and I'll follow up with adjustments.

@gsmet
Copy link
Member

gsmet commented May 29, 2019

@aloubyansky we have time. See my email on the list. I'll release tomorrow or Friday.

@aloubyansky
Copy link
Member

Yes, just saw it. Ok. I'll wait a bit for @jycr to respond.

@jycr
Copy link
Contributor Author

jycr commented May 29, 2019

Yes, just saw it. Ok. I'll wait a bit for @jycr to respond.

@aloubyansky : I do not see what remains to be done.
Have I forgotten something?

@aloubyansky
Copy link
Member

Oh, you've already pushed! Sorry, I missed it. Merging... Thanks!

@aloubyansky
Copy link
Member

well, waiting for the CI first

@aloubyansky aloubyansky added the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 29, 2019
@jycr
Copy link
Contributor Author

jycr commented May 29, 2019 •
edited
Loading

well, waiting for the CI first

The CI has just finished 🙂
(NB: builds are very long ... is this normal?)

@aloubyansky aloubyansky merged commit 0a8c173 into quarkusio:master May 29, 2019
@jycr jycr deleted the bugfix/2470 branch May 29, 2019 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aloubyansky aloubyansky aloubyansky approved these changes

@gsmet gsmet Awaiting requested review from gsmet

Assignees
No one assigned
Labels
triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Milestone
0.16.0
Development

Successfully merging this pull request may close these issues.

Quarkus bypasses Nexus and downloads artefacts directly from maven central
3 participants
@jycr @aloubyansky @gsmet