-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix: 7093 add username/password properties to be able to authenticate for central.content.url and analyzer.central.url again #7169
fix: 7093 add username/password properties to be able to authenticate for central.content.url and analyzer.central.url again #7169
Conversation
… for central.content.url and analyzer.central.url again
Thanks for the PR. As it adds CLI commandline arguments It would be worthwhile to also add the same configurability to the maven plugin and Ant task (and the gradle plugin, which is maintained in a different repo), but I'm perfectly fine in enhancing those as separate PRs |
I have added the two arguments that are added to the cli to the arguments.md as requested |
@drijkersbq Overlooked in my initial review, but spotted it after you added the documentation: In the settings you introduce properties to override user/pass for search as well as for content. But in the CLI arguments you only add a way to override the credentials for search. Intentional? Or accidental because credentials and host happen to be identical in your case so that defining them once makes both cases succeed for you? If host and port are the same one set of creds would replace the other, as the credentials put in a map linked to host/port combo, which means that if both are the same for search- and content-URL it would likely go unnoticed that only one of the two was attempted to be configured (unless the proxy host was expecting different credentials for search vs content paths, in which case one of the two would still fail, but that would be an unsupported case with the current codebase anyhow given that neither URI-paths nor authentication-realms play a role in the current configs for credentials) |
@aikebah The reason why i've only added CLI arguments for just one of the two url's is because only one of those two url's was currently available as a CLI argument itself. The second url was not yet available as cli argument. Not knowing why the second url wasnt available as cli argument already (perhaps there was a good reason for it), i didnt it's new credential properties to the cli. It's true in our case both url's are thesame so we could get away with only one url+credentials-set in the cli arguments. However we run the project with a custom property file where we set the corresponding properties for both url's, so we dont use the cli arguments for either of the urls. If you want i can add the second url and its new credential properties to the cli arguments to match up the first url. |
@drijkersbq No change needed, would be fine as is... was reviewing on iPad, so not easy to cross-check on codebase. Your explanation makes sense. LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@jeremylong would appreciate your review as well before merging it in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fix: add username/password properties to be able to authenticate for central.content.url and analyzer.central.url again
Fixes Issue
7093
Description of Change
Added username and password properties for authentication of both the central.content.url and analyzer.central.url:
This fixes the problem that the new httpclient doesn't support userinfo components in these url's anymore.
Have test cases been added to cover the new functionality?
no