You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by bostko <gi...@git.apache.org> on 2016/04/20 18:17:01 UTC
[GitHub] brooklyn-server pull request: Add method HttpFeed.Builder#credenti...
GitHub user bostko opened a pull request:
https://github.com/apache/brooklyn-server/pull/119
Add method HttpFeed.Builder#credentials(Credentials)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bostko/brooklyn-server http_feed_add_password
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/119.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #119
----
commit 021953d4220ea00875d0a736df2b15db190685db
Author: Valentin Aitken <bo...@gmail.com>
Date: 2016-04-20T16:16:40Z
Add method HttpFeed.Builder#credentials(Credentials)
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #119: Add method HttpFeed.Builder#credentials(Credenti...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/119
Bump @bostko.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: Add method HttpFeed.Builder#credenti...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the pull request:
https://github.com/apache/brooklyn-server/pull/119#issuecomment-222327645
An example is when you want to provide the HttpFeed with a mutable UsernameAndPasswordCredentials. When the credentials are changed you should it is useful to be able to alter the credentials object from code who is aware of the credentials change. Example https://github.com/brooklyncentral/brooklyn-ambari/pull/101/files
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #119: Add method HttpFeed.Builder#credentials(Credenti...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/119
@bostko Ah, thanks for explaining the use-case. Could we instead fix that with an overloaded method:
public Builder credentials(Supplier<String> username, Supplier<String> password) { ...
Under-the-covers, that could either instantiate a mutable `UsernameAndPasswordCredentials`.
We'd need to be careful with a mutable credentials object. I'm not sure how well `HttpClient` will cope with being instantiated with mutable credentials. Hopefully it would be ok; I've never tried. (we create the `HttpClient` in `HttpFeed.preStart()`, and then re-use it for each poll.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: Add method HttpFeed.Builder#credenti...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/brooklyn-server/pull/119#issuecomment-222312481
@bostko any comments on this? Do you agree that we shouldn't expose `o.a.h.a.Credentials` in the API? What is the use-case?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #119: Add method HttpFeed.Builder#credentials(Credenti...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the issue:
https://github.com/apache/brooklyn-server/pull/119
Closing it until it is really needed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #119: Add method HttpFeed.Builder#credentials(Credenti...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the issue:
https://github.com/apache/brooklyn-server/pull/119
@neykov what are your thoughts on this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: Add method HttpFeed.Builder#credenti...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/119#discussion_r62225338
--- Diff: core/src/main/java/org/apache/brooklyn/feed/http/HttpFeed.java ---
@@ -197,6 +197,10 @@ public Builder credentials(String username, String password) {
this.credentials = new UsernamePasswordCredentials(username, password);
return this;
}
+ public Builder credentials(Credentials credentials) {
--- End diff --
Not sure about this. Nothing in the HttpFeed's public API talks about `org.apache.http.auth`. If we add this public method, then `org.apache.http.auth.Credentials` is exposed (rather than it being an implementation detail).
Mind you, some of our other "public" classes (e.g. `HttpTool`) exposes `org.apache.http.*` in its API.
What is the use-case for adding this?
@rdowner what are your thoughts - should we expose this in the API?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #119: Add method HttpFeed.Builder#credentials(C...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/119#discussion_r77529695
--- Diff: core/src/main/java/org/apache/brooklyn/feed/http/HttpFeed.java ---
@@ -197,6 +197,10 @@ public Builder credentials(String username, String password) {
this.credentials = new UsernamePasswordCredentials(username, password);
return this;
}
+ public Builder credentials(Credentials credentials) {
--- End diff --
Agree that we should avoid it if possible. Depending on the underlying implementation being mutable and changing it **after** being passed (that's how I understand the change) doesn't sound like a good idea.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #119: Add method HttpFeed.Builder#credentials(C...
Posted by bostko <gi...@git.apache.org>.
Github user bostko closed the pull request at:
https://github.com/apache/brooklyn-server/pull/119
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #119: Add method HttpFeed.Builder#credentials(Credenti...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/119
Agree with @aledsage. Do we really need to marry our API to http client? A mutable credentials object depends on implementation details.
Why need this, what's the driver?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: Add method HttpFeed.Builder#credenti...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/brooklyn-server/pull/119#issuecomment-218985333
@bostko i agree w @aledsage, it's nicer if we can avoid putting `o.a.h.a.Credentials` in the api: what is the use case? maybe a use-case specific method would be better.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---