You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ahgittin <gi...@git.apache.org> on 2014/12/17 16:24:52 UTC
[GitHub] incubator-brooklyn pull request: support URL for the keystore supp...
GitHub user ahgittin opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/403
support URL for the keystore supplied to brooklyn web server
(since we do call it keystore url!)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/incubator-brooklyn support-url-for-keystore
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/403.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 #403
----
commit 96426621ac363090a8297b19209dca368bb64223
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2014-12-17T15:24:17Z
support URL for the keystore supplied to brooklyn web server
(since we do call it keystore url!)
----
---
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] incubator-brooklyn pull request: support URL for the keystore supp...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/403#discussion_r22468141
--- Diff: usage/launcher/src/main/java/brooklyn/launcher/BrooklynWebServer.java ---
@@ -364,19 +367,28 @@ public synchronized void start() throws Exception {
SslContextFactory sslContextFactory = new SslContextFactory();
- if (keystorePath==null) keystorePath = managementContext.getConfig().getConfig(BrooklynWebConfig.KEYSTORE_URL);
+ // allow webconsole keystore & related properties to be set in brooklyn.properties
+ if (Strings.isNonBlank(keystorePath)) {
+ if (keystoreUrl==null) {
+ log.warn("Deprecated 'keystorePath' used; callers should use 'keystoreUrl'");
+ keystoreUrl = keystorePath;
+ } else if (!keystoreUrl.equals(keystorePath)) {
+ log.warn("Deprecated 'keystorePath' supplied with different value than 'keystoreUrl', preferring the latter: "+
--- End diff --
If both `keystoreUrl` and `keystorePath` are given I think it would be better to throw rather than to log and ignore the latter. The warning message will be missed, it will not be obvious which property will be used when reading property files and we will end up with confused users.
---
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] incubator-brooklyn pull request: support URL for the keystore supp...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/403#issuecomment-70709642
What is the entry point for the scenario you describe? Deploying a `BrooklynNode`?
---
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] incubator-brooklyn pull request: support URL for the keystore supp...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/403#issuecomment-71164205
k thx - merging
---
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] incubator-brooklyn pull request: support URL for the keystore supp...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/403#issuecomment-70951233
@sjcorbett `BrooklynNode` could be one way, or a custom launcher. unlikely to be a real issue, if you still feel it should fail fast i'll change it.
---
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] incubator-brooklyn pull request: support URL for the keystore supp...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/403#issuecomment-71100096
they wouldn't get exceptions in the future would they? it's just that if they supply different `url` and `path` values i say `url` should trump with a warning now, you say it should fail now. in the future when `path` is removed `url` will trump with no warning or exception, `path` will just be ignored.
this is a general case of we deprecate property `A` and replace it with property `B`. what happens if `A` and `B` are supplied and disagree. failing in this case seems harsh it means anyone interacting with a legacy library which sets `A` would themselves have to use `A` in order to make it work. better they just use `B` and get a warning about `A` being used by a legacy library.
---
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] incubator-brooklyn pull request: support URL for the keystore supp...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/403#issuecomment-70624114
Agree with @sjcorbett - better to throw exception during startup. Other than that, looks good.
---
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] incubator-brooklyn pull request: support URL for the keystore supp...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/403#issuecomment-71029622
I guess it boils down to whether we want users to get exceptions now or at some point in the future when the deprecated property is removed. I'd prefer the former but am not going to fuss if you know of users it would affect immediately.
---
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] incubator-brooklyn pull request: support URL for the keystore supp...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/403#issuecomment-71101839
You're right with regard to behaviour. I think I'd just confused myself earlier.
Not sure I follow your second paragraph. Either way, my original suggestion had standalone-server startup in mind. It's not that harsh throwing in that case because it'll happen within seconds of running `brooklyn launch`. I agree that throwing is less desirable if a `BrooklynNode` entity is launched as part of a larger deployment. I defer to you based on your opinion of how likely the latter case is.
---
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] incubator-brooklyn pull request: support URL for the keystore supp...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/403
---
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] incubator-brooklyn pull request: support URL for the keystore supp...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/403#issuecomment-70644055
disagree. if someone sets `path` and `url` to `X` and someone elase composes with that blueprint it should be sufficient for them to set just `url` to `Y` and have that take effect. they shouldn't have to also go and set the deprecated parameter.
this is what warnings are for -- use and abuse of deprecated parameters, when there is clearly a dominant setting.
okay to merge?
---
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.
---