Skip to content

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?

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

@ivanayov ivanayov force-pushed the generate-config-from-yaml branch from 186c175 to aab2fc9 Compare January 29, 2016 10:35
@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)

@ivanayov ivanayov closed this Feb 1, 2016
@ivanayov ivanayov reopened this Feb 1, 2016
@ivanayov ivanayov force-pushed the generate-config-from-yaml branch from aab2fc9 to 7774e46 Compare February 1, 2016 09:14
@ivanayov ivanayov closed this Feb 1, 2016
@ivanayov ivanayov reopened this Feb 1, 2016
@ivanayov ivanayov force-pushed the generate-config-from-yaml branch from 7774e46 to e6d8fb2 Compare February 1, 2016 14:03
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