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.

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

Multiple proxy settings in maven settings breaks the plugin function. #1495

Merged
merged 5 commits into from
Sep 21, 2018

Conversation

tyagiakhilesh
Copy link
Contributor

@tyagiakhilesh tyagiakhilesh commented Sep 21, 2018

Fixes Issue #831

Description of Change

In case we find multiple settings for proxy in maven settings, pick the first active one. Also, it might be very much possible that proxy definition is specified in maven settings file but they are not active. Hence throwing IllegalStateException is not really correct?! Hence letting null return in case none of the proxy settings are active.

Have test cases been added to cover the new functionality?

I would love to. But not able to figure out where are the relevant test cases for this at the moment. Hence I going ahead and raising this PR. Let us see if any test case fails for this.

…ings.

In case we find multiple settings for proxy in maven settings, pick the first
found https one. Next pick http one. Only active proxies should be picked up.
If there are no active proxies found, then simply fall back on existing state
exception.
…ings.

Since we are looping over the proxies, being consistent in picking proxy make
more sense rather then picking only first one found. Also, it might be very much
possible that proxy definition is specified in maven settings file but they are not
active. Hence throwing IllegalStateException is not really correct?! Hence letting
null return in case none of the proxy settings are active.
@tyagiakhilesh
Copy link
Contributor Author

Tested the plugin with this in my settings.xml and things are working good. Can we please take this up for review?

<proxies>
    <proxy>
      <id>http</id>
      <active>false</active>
      <protocol>http</protocol>
      <host>xxxxx.xx.xxxxxxx.net</host>
      <port>XXXX</port>
      <nonProxyHosts>local.net|some.host.com</nonProxyHosts>
    </proxy>
    <proxy>
      <id>https</id>
      <active>true</active>
      <protocol>https</protocol>
      <host>xxxxx.xx.xxxxxxx.net</host>
      <port>XXXX</port>
      <nonProxyHosts>local.net|some.host.com</nonProxyHosts>
    </proxy>
  </proxies>

@jeremylong
Copy link
Owner

Can one of the admins verify this patch?

@tyagiakhilesh
Copy link
Contributor Author

Tested again with my latest changes. Working good.

@jeremylong jeremylong merged commit 8190a86 into jeremylong:master Sep 21, 2018
@lock lock bot locked and limited conversation to collaborators Apr 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants