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

Add support for kubernetes helm #830

Merged
merged 4 commits into from
Apr 11, 2018

Conversation

andreaturli
Copy link
Contributor

No description provided.

@andreaturli
Copy link
Contributor Author

retest this please

@tbouron
Copy link
Member

tbouron commented Nov 22, 2017

@andreaturli Might be completely off-base here but wouldn't it make more sense for this to be in the clocker project

@andreaturli
Copy link
Contributor Author

good question @tbouron -- I think Helm is a package manager for Kubernets and helps you deploy application to kubernetes. Seemed to me the location/container is a good fit, as it is where brooklyn implements the support to deployTo kubernetes. No strong feelings though

Copy link
Member

@tbouron tbouron left a comment

Choose a reason for hiding this comment

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

Fair enough @andreaturli. LGTM from a code POV. Is there any way I can test this myself?

<!--<exclusion>-->
<!--<groupId>io.grpc</groupId>-->
<!--<artifactId>grpc-netty</artifactId>-->
<!--</exclusion>-->
Copy link
Member

Choose a reason for hiding this comment

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

Can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @tbouron
also now there is an official release http://search.maven.org/#search%7Cga%7C1%7Ca%3Amicrobean-helm which may be better and needs testing

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, I'll let you decide which one is best then ;)

<!--<exclusion>-->
<!--<groupId>com.google.instrumentation</groupId>-->
<!--<artifactId>instrumentation-api</artifactId>-->
<!--</exclusion>-->
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

@@ -347,8 +386,8 @@ public Boolean call() {
if (result.isEmpty()) {
return false;
}
List<HasMetadata> check = client.resource(result.get(0)).inNamespace(result.get(0).getMetadata().getNamespace()).get();
if (result.size() > 1 || check.size() != 1 || check.get(0).getMetadata() == null) {
HasMetadata check = client.resource(result.get(0)).inNamespace(result.get(0).getMetadata().getNamespace()).get();
Copy link
Member

Choose a reason for hiding this comment

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

Is that normal the signature changed from List<HasMetadata> to HasMetadata ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

think so, but need to double-check as I don't recall the details

@andreaturli
Copy link
Contributor Author

@tbouron I remember I used minikube to test it. Any k8s cluster should work though

@ahgittin
Copy link
Contributor

would be great to have this merged! nearly there IMO.

re clocker, deploy to i think of as Brooklyn, and this is squarely in that camp ... deploy of {k8s,docker,etc} is more clocker.

would be nice to have a yaml example -- ideally a docs addition that references an example tested here (is there a brooklyn-docs PR ?). also nice to have another pair of eyes on this but given the time and value, and i've looked at the code and am happy with it, i'm happy for this to be merged.

@andreaturli andreaturli force-pushed the feature/container-k8s-helm branch from 209be64 to 42ea31c Compare February 16, 2018 11:39
@andreaturli andreaturli force-pushed the feature/container-k8s-helm branch 2 times, most recently from 0a78073 to 96015e6 Compare March 12, 2018 08:16
- bump microbean-helm version
- support default stable ChartRepository
- use auto-generated release name
@andreaturli andreaturli force-pushed the feature/container-k8s-helm branch from 96015e6 to 6ddc595 Compare March 12, 2018 10:06
@andreaturli
Copy link
Contributor Author

retest this please

andreaturli added a commit to andreaturli/brooklyn-dist that referenced this pull request Mar 26, 2018
@andreaturli
Copy link
Contributor Author

andreaturli commented Apr 11, 2018

@tbouron as per our review, I've add the creation of some .helm dirs required to run on a fresh installation: thanks for spotting the issue!

it basically mimics the same approach taken by the microbeam-helm project - https://github.com/microbean/microbean-helm/blob/ae51ed6571af204df9b764f0b2b59f2f8e2c2aa4/.travis.yml#L6

@tbouron
Copy link
Member

tbouron commented Apr 11, 2018

Just tested, works like a charm, thanks @andreaturli !

Just need to make Jenkins happy again and I'll merge it 👍

@tbouron
Copy link
Member

tbouron commented Apr 11, 2018

retest this please

@asfgit asfgit merged commit d091f82 into apache:master Apr 11, 2018
asfgit pushed a commit that referenced this pull request Apr 11, 2018
asfgit pushed a commit that referenced this pull request Apr 16, 2018
This reverts commit 0debf39, reversing
changes made to 12e6b00.
duncangrant pushed a commit to duncangrant/brooklyn-server that referenced this pull request Apr 29, 2020
Revert "Revert "This closes apache#830""

This reverts commit 4236ff2.
iuliana pushed a commit to iuliana/brooklyn-server that referenced this pull request May 18, 2020
Revert "Revert "This closes apache#830""

This reverts commit 4236ff2.
iuliana pushed a commit to iuliana/brooklyn-server that referenced this pull request May 18, 2020
Revert "Revert "This closes apache#830""

This reverts commit 4236ff2.
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.

4 participants