You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by tbouron <gi...@git.apache.org> on 2017/01/05 15:56:45 UTC
[GitHub] brooklyn-server pull request #504: Make the brooklyn server config discovera...
GitHub user tbouron opened a pull request:
https://github.com/apache/brooklyn-server/pull/504
Make the brooklyn server config discoverable by OSGi and downstream projects
This is a follow up PR of https://github.com/apache/brooklyn-dist/pull/72
It moves the brooklyn server config back into `brooklyn-server` and create a feature that will be used in `brooklyn-dist`
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tbouron/brooklyn-server feature/brooklyn-server-config
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/504.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 #504
----
commit a828901348611e669e763c525f19d9dfb702ed06
Author: Thomas Bouron <th...@cloudsoftcorp.com>
Date: 2017-01-05T15:54:32Z
Make the brooklyn server config discoverable by OSGi and downstream projects
----
---
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 #504: Make the brooklyn server config discovera...
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/504#discussion_r95331819
--- Diff: karaf/config/pom.xml ---
@@ -0,0 +1,74 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <parent>
+ <artifactId>brooklyn-karaf</artifactId>
+ <groupId>org.apache.brooklyn</groupId>
+ <version>0.11.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>brooklyn-server-config</artifactId>
+
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.codehaus.mojo</groupId>
+ <artifactId>build-helper-maven-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>attach-artifact</id>
+ <phase>package</phase>
+ <goals>
+ <goal>attach-artifact</goal>
+ </goals>
+ <configuration>
+ <artifacts>
+ <artifact>
+ <file>
+ ${project.basedir}/src/main/resources/org.apache.brooklyn.classrename.cfg
+ </file>
+ <type>cfg</type>
+ <classifier>classrename</classifier>
+ </artifact>
+ <artifact>
+ <file>
+ ${project.basedir}/src/main/resources/org.apache.brooklyn.core.catalog.bomscanner.cfg
+ </file>
+ <type>cfg</type>
+ <classifier>core.catalog.bomscanner</classifier>
--- End diff --
We don't use "core" in any of the other names. Maybe we shouldn't use "core" here either, and instead just name it "catalog.bomscanner".
Also, are there any tests that this works (not that it's necessarily this PR's job to add the tests for the pre-existing config!)?
---
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 #504: Make the brooklyn server config discoverable by ...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on the issue:
https://github.com/apache/brooklyn-server/pull/504
Well, jenkins won't be happy for https://github.com/apache/brooklyn-dist/pull/75 until this PR is merged (it is uses the new feature created here)
---
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 #504: Make the brooklyn server config discoverable by ...
Posted by m4rkmckenna <gi...@git.apache.org>.
Github user m4rkmckenna commented on the issue:
https://github.com/apache/brooklyn-server/pull/504
@neykov My personal pref is to keep the config files in the same repository as the code that uses it ... I hate PRs that span multiple repositories but i guess its not that big a deal
---
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 #504: Make the brooklyn server config discoverable by ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/504
My reasoning is:
* This is runtime configuration and as such is defined by the requirements of the distribution the files end up in.
* That's analogous to `default.catalog.bom` - each distribution has its own needs of what goes in it - it's not the Brooklyn "framework" that decides what's in the catalog - it just consumes it.
* When in brooklyn-dist, could pack all the config files together to be re-used.
That said, both approaches are technically correct and this is turning into bike-shedding discussion so I'm happy with any solution.
---
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 #504: Make the brooklyn server config discoverable by ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/504
I take it we're happy to leave the itests (e.g. https://github.com/apache/brooklyn-dist/blob/master/karaf/itest/src/test/java/org/apache/brooklyn/core/catalog/internal/CatalogBomScannerTest.java) in brooklyn-dist, even though it's brooklyn-server that declares the config files? I think it needs the full `brooklyn-dist` bulid in order to run those itests.
---
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 #504: Make the brooklyn server config discovera...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/504#discussion_r95333222
--- Diff: karaf/config/src/main/resources/org.apache.brooklyn.core.catalog.bomscanner.cfg ---
@@ -0,0 +1,27 @@
+################################################################################
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+################################################################################
+
+# CSV Whitelist of regular expressions to match bundle symbolic id if bundle is to be permitted to add
--- End diff --
I think it should be addressed in another PR
---
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 #504: Make the brooklyn server config discoverable by ...
Posted by drigodwin <gi...@git.apache.org>.
Github user drigodwin commented on the issue:
https://github.com/apache/brooklyn-server/pull/504
I don't think we can merge this and [brooklyn-dist/75](https://github.com/apache/brooklyn-dist/pull/75) in it's current state as it will break the classic launch brooklyn.
---
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 #504: Make the brooklyn server config discoverable by ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/504
Re itests - that's correct @aledsage, the only reason to have them here is that we need the karaf dist built before running them.
Slight preference to keeping all the config files together with the (possibly any downstreams) assembly dist cherry-picking 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 pull request #504: Make the brooklyn server config discovera...
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/504#discussion_r95330255
--- Diff: karaf/config/src/main/resources/org.apache.brooklyn.core.catalog.bomscanner.cfg ---
@@ -0,0 +1,27 @@
+################################################################################
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+################################################################################
+
+# CSV Whitelist of regular expressions to match bundle symbolic id if bundle is to be permitted to add
--- End diff --
Unrelated to this PR (because you're just moving the files from brooklyn-dist), but we should probably include a comment that says about `brooklyn.experimental.feature.osgi.catalog_bom=true`. Without enabling that functionality, this file will presumably have no effect.
See https://github.com/apache/brooklyn-server/blob/master/core/src/main/java/org/apache/brooklyn/core/BrooklynFeatureEnablement.java#L122-L127
---
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 #504: Make the brooklyn server config discoverable by ...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on the issue:
https://github.com/apache/brooklyn-server/pull/504
Ok, looks like this is controversial so let's change nothing, it will be simpler and quicker.
I'll close this PR, re-add the files in `brooklyn-dist` on https://github.com/apache/brooklyn-dist/pull/75 and do the feature changes over there.
---
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 #504: Make the brooklyn server config discoverable by ...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on the issue:
https://github.com/apache/brooklyn-server/pull/504
Good catch @drigodwin, the classic mode would not work without the `default.catalog.bom` in `brooklyn-server`.
I re-added this file in https://github.com/apache/brooklyn-dist/pull/75 and removed it here, should be fine now.
---
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 #504: Make the brooklyn server config discovera...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron closed the pull request at:
https://github.com/apache/brooklyn-server/pull/504
---
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 #504: Make the brooklyn server config discoverable by ...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on the issue:
https://github.com/apache/brooklyn-server/pull/504
@neykov Same as @m4rkmckenna, I think it's cleaner to have the config files in the same repo as the code that use them. Now I can always revert back, close this PR and do all this in `brooklyn-dist`.
What do you guys think?
---
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 #504: Make the brooklyn server config discovera...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/504#discussion_r95333103
--- Diff: karaf/config/pom.xml ---
@@ -0,0 +1,74 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <parent>
+ <artifactId>brooklyn-karaf</artifactId>
+ <groupId>org.apache.brooklyn</groupId>
+ <version>0.11.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>brooklyn-server-config</artifactId>
+
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.codehaus.mojo</groupId>
+ <artifactId>build-helper-maven-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>attach-artifact</id>
+ <phase>package</phase>
+ <goals>
+ <goal>attach-artifact</goal>
+ </goals>
+ <configuration>
+ <artifacts>
+ <artifact>
+ <file>
+ ${project.basedir}/src/main/resources/org.apache.brooklyn.classrename.cfg
+ </file>
+ <type>cfg</type>
+ <classifier>classrename</classifier>
+ </artifact>
+ <artifact>
+ <file>
+ ${project.basedir}/src/main/resources/org.apache.brooklyn.core.catalog.bomscanner.cfg
+ </file>
+ <type>cfg</type>
+ <classifier>core.catalog.bomscanner</classifier>
--- End diff --
@aledsage This PR just make the configuration files available through karaf features. If you want to changes names I think it should be addressed in another PR, especially because it won't be backward compatible.
---
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.
---