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

Fix Helm binding errors #359

Merged
merged 5 commits into from
Sep 12, 2019
Merged

Conversation

yoshi-1224
Copy link
Contributor

@yoshi-1224 yoshi-1224 commented Sep 3, 2019

This PR aims to fix all the helm binding related issues.

Shifting from alpine docker image (fixes #358)

The Dockerfile in this PR is changed to FROM python:3.7-slim-stretch. This was chosen because it is one of the smaller debian images which have python 3.7 installed by default (the custom image is named as kapitan-debian below).

$ docker image ls
REPOSITORY           TAG                 IMAGE ID            CREATED             SIZE
kapitan-debian       latest              728041763c54        2 hours ago         1.04GB
deepmind/kapitan     latest              0670fbc69cde        4 days ago          860MB
python               3.7-alpine          39fb80313465        7 days ago          98.7MB
python               3.7-slim-stretch    7f8ea8958a02        7 days ago          155MB

I think the image size difference is not so bad (~ 160MB) given that musl-libc is going to be smaller than glibc to start with.

So far the image is working just fine (tested with the examples folder, plus helm binding works as well). Got decryption failed error when using --reveal option but I also get that using deepmind/kapitan image so I may just be doing it wrongly.

If this is acceptable, we can move on to change the base image of Dockerfile.ci as well.

Supporting subcharts

The binding should now correctly work with charts with subcharts

@yoshi-1224 yoshi-1224 changed the title Shift to debian image Fix Helm binding errors Sep 3, 2019
@ademariag
Copy link
Contributor

I think we should also fix Dockerfile.ci

Copy link
Contributor

@uberspot uberspot left a comment

Choose a reason for hiding this comment

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

LGTM! I'd just add the same changes to Dockerfile.ci as well so that everything is on the same page.
But it can also be done in another PR too, up to you. 👍

kapitan/inputs/helm/__init__.py Show resolved Hide resolved
kapitan/inputs/helm/__init__.py Show resolved Hide resolved
kapitan/inputs/helm/template.go Show resolved Hide resolved
@yoshi-1224
Copy link
Contributor Author

@uberspot
Should we change the name of the helm binding shared object according to which platform this is targeted, just as we did for kapitan binary? In my understanding @ramaro wants the so distributed for each of Linux and Mac. I'm working on cross compile for OSX now, which hopefully takes us somewhere

@uberspot
Copy link
Contributor

uberspot commented Sep 4, 2019

@uberspot
Should we change the name of the helm binding shared object according to which platform this is targeted, just as we did for kapitan binary? In my understanding @ramaro wants the so distributed for each of Linux and Mac. I'm working on cross compile for OSX now, which hopefully takes us somewhere

Sure, go for it!
Just don't forget to update the docs too if there's any mention of the .so there too. :)

@ademariag
Copy link
Contributor

Is there an update on this?

@yoshi-1224
Copy link
Contributor Author

Hi @ademariag
Sorry I'd been busy and couldn't really get on this task until now.

@uberspot
Could you please have a look at the new the docker CI image and confirm whether everything that was there on the previous alpine version is available as before?

Dockerfile.ci Outdated Show resolved Hide resolved
Copy link
Contributor

@taharah taharah left a comment

Choose a reason for hiding this comment

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

Everything else looks good to me thus far.

Dockerfile Show resolved Hide resolved
@yoshi-1224
Copy link
Contributor Author

@uberspot
what would be the most convenient way to build and distribute helm binding for Mac in addition to the existing Linux binding? Is cross-compiling on Travis the way, or do you think there might be an easier path (e.g. is it possible to build natively on OSX machine on Travis from which we can copy the built shared object file onto our Debian one)?

@ademariag
Copy link
Contributor

May I suggest that we look at the helm binding in mac in another PR? I feel this one if getting more than it came for. This way we can unlock testing on the current state.

@yoshi-1224
Copy link
Contributor Author

@ademariag
I totally agree. That way @taharah can get started on the multistage Docker image(s) too. @uberspot Let me work on OSX binding in another PR. I'm going to make this one ready for review.

@yoshi-1224 yoshi-1224 marked this pull request as ready for review September 12, 2019 00:17
Copy link
Contributor

@uberspot uberspot left a comment

Choose a reason for hiding this comment

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

LGTM!

@uberspot
Copy link
Contributor

@uberspot
what would be the most convenient way to build and distribute helm binding for Mac in addition to the existing Linux binding? Is cross-compiling on Travis the way, or do you think there might be an easier path (e.g. is it possible to build natively on OSX machine on Travis from which we can copy the built shared object file onto our Debian one)?

About this btw, https://docs.travis-ci.com/user/multi-os/ https://docs.travis-ci.com/user/reference/osx/ these two docs would cover how to run a specific test to build the OSX bindings in a different env too.

Merging this MR for now, and let's keep the OSX work in a separate PR. :)
Nice work!

@uberspot uberspot merged commit 9932ba0 into kapicorp:master Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants