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

Generate config/effector descriptions for yaml-only blueprints #1176

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ivanayov
Copy link
Contributor

No description provided.

@tbouron
Copy link
Member

tbouron commented Jan 27, 2016

LGTM.

One comment though. This generate a JS file which encapsulates the JSON data into a JS var. While this is ok for the documentation as it imports the file directly, it's a bit tedious to share the info with other services (i.e. community catalog) as we will need to eval this file to get the data.
I know it was not introduced by this PR but would it be better to get this opportunity to change the format from JS to JSON?

.build();
jsonList.add(toJson(result));
} else if (!yaml.isEmpty()) {
LocalManagementContext lmgmt = new LocalManagementContext();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will read config from the user's brooklyn.properties. Better initialize it with an empty state using new LocalManagementContext(BrooklynProperties.Factory.newEmpty());

@ivanayov ivanayov force-pushed the generate-config-from-yaml branch 3 times, most recently from 782e2b7 to 186c175 Compare January 28, 2016 15:23
@ivanayov
Copy link
Contributor Author

Jenkins fails due to DynamicSequentialTaskTest.testCancelled which is not related to the issue.
The test is successful locally.

@bostko
Copy link
Contributor

bostko commented Jan 29, 2016

@tbouron @neykov if you had noticed docs were loading from a json file for a while.
However I noticed the experience wasn't so smooth on production and reverted it back.
When it is js file in the html it is automatically cached by the browser with no extra code.
#622

@tbouron
Copy link
Member

tbouron commented Jan 29, 2016

@bostko Didn't notice that. Let's not reintroduce a slow process then.

However, I quite like the @iyovcheva's idea of generate the JS and JSON file. That will be much more safer and easier for the community catalog :)

@ahgittin
Copy link
Contributor

test failure is clearly unrelated, maybe close and reopen to run again.

meanwhile we should look at this /cc @aledsage :

org.apache.brooklyn.entity.software.base.test.autoscaling.AutoScalerPolicyNoMoreMachinesTest.testResizeDirectly (from TestSuite)

java.lang.AssertionError: removed=[EmptySoftwareProcessImpl{id=NhzxGta6}] expected [2] but found [1]
    at org.testng.Assert.fail(Assert.java:94)
    at org.testng.Assert.failNotEquals(Assert.java:494)
    at org.testng.Assert.assertEquals(Assert.java:123)
    at org.testng.Assert.assertEquals(Assert.java:370)
    at org.apache.brooklyn.entity.software.base.test.autoscaling.AutoScalerPolicyNoMoreMachinesTest.assertSize(AutoScalerPolicyNoMoreMachinesTest.java:182)
    at org.apache.brooklyn.entity.software.base.test.autoscaling.AutoScalerPolicyNoMoreMachinesTest.testResizeDirectly(AutoScalerPolicyNoMoreMachinesTest.java:105)

@ahgittin
Copy link
Contributor

(easily fixed - the test was doing an immediate assert on something happening in another thread; PR to follow; @iyovcheva give it an hour or so for the other fix to land then please close+reopen this to run the jenkins tests again)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants