Skip to content

Update README examples#31

Merged
neykov merged 1 commit intomasterfrom
update-readme
May 30, 2017
Merged

Update README examples#31
neykov merged 1 commit intomasterfrom
update-readme

Conversation

@bostko
Copy link
Copy Markdown
Contributor

@bostko bostko commented May 27, 2017

No description provided.

Copy link
Copy Markdown
Contributor

@neykov neykov left a comment

Choose a reason for hiding this comment

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

Looks good, could improve karaf instructions as seems the updates are targeted at karaf.

Comment thread README.md Outdated
customizers:
- $brooklyn:object:
type: brooklyn.location.blockstore.ec2.Ec2NewVolumeCustomizer
type: io.brooklyn.blockstore.brooklyn-blockstore:brooklyn.location.blockstore.NewVolumeCustomizer
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The changes here assume Karaf distribution, while the instructions above still document how to add dependencies to the classic distribution.
Ideally this should be added as a catalog item with catalog.libraries pointing to the blockstore jar. Alternatively could instruct the user to copy to deploy folder.
Do you think above instructions should be updated for karaf?

Comment thread README.md
@@ -90,15 +113,15 @@ This module should be built as an OSGi bundle, so that it can more easily be add
The API is under review, particularly to make it easier to use with YAML blueprints.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A couple of lines above:

 This module should be built as an OSGi bundle, so that it can more easily be added to Brooklyn.

This should already be the case.

@neykov
Copy link
Copy Markdown
Contributor

neykov commented May 29, 2017

Good to merge, karaf doc updates can be done in another PR.

@bostko bostko force-pushed the update-readme branch 2 times, most recently from 7a4ee72 to 4ccec74 Compare May 29, 2017 15:51
Comment thread README.md
This project builds on the open-source project Apache Brooklyn (see
http://brooklyn.apache.org and https://github.com/apache/brooklyn).
git clone git@github.com:cloudsoft/brooklyn-blockstore.git
mvn clean install
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cd brooklyn-blockstore

Copy link
Copy Markdown
Contributor

@neykov neykov left a comment

Choose a reason for hiding this comment

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

Some small changes to the examples needed, otherwise looks good.

Comment thread README.md Outdated
The example below creates a VM with a new volume:

brooklyn.catalog:
brooklyn.catalog:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Double brooklyn.catalog.

Comment thread README.md Outdated
brooklyn.catalog:
brooklyn.catalog:
id: my-app.io.cloudsoft.brooklyn-blockstore.new-volume-customizer
version: 0.6.0-SNAPSHOT
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tag the version so its updated when bumping to a new version. Use BROOKLYN_BLOCKSTORE_VERSION, I think html comments will hide it for the user.

Comment thread README.md Outdated
id: my-app.io.cloudsoft.brooklyn-blockstore.new-volume-customizer
version: 0.6.0-SNAPSHOT
brooklyn.libraries:
- file://home/myUser/repos/cloudsoft/brooklyn-blockstore/target/brooklyn-blockstore-0.6.0-SNAPSHOT.jar
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tag the version here as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the instructions recommend building locally, better use ~/.m2/repository/.....

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@neykov does Apache Brooklyn support ~ for file:// catalog libraries?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, I think it will recognise it.

Comment thread README.md Outdated
mountPoint: /mount/brooklyn/h
filesystemType: ext4
brooklyn.catalog:
brooklyn.catalog:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same comments as above snippet - double catalog, tag the version.

Comment thread README.md Outdated
filesystemType: ext4
cd ${BROOKLYN_BLOCKSTORE_REPO}

echo Follow Apache Brooklyn Getting Started instructions from https://brooklyn.apache.org/#get-started
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Section above (3 lines shell script) no longer useful, remove altogether.

@bostko
Copy link
Copy Markdown
Contributor Author

bostko commented May 30, 2017

@neykov I addressed you comments.
However I tested including it in Apache Brooklyn karaf and failed due to cinder dependency :(
I will consider removing it in a following PR since this is an old dependency nobody uses.

@neykov neykov merged commit 9d5cdb4 into master May 30, 2017
@neykov neykov deleted the update-readme branch May 30, 2017 14:38
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.

2 participants