kapicorp / kapitan

Generic templated configuration management for Kubernetes, Terraform and other things
https://kapitan.dev
Apache License 2.0
1.83k stars 199 forks source link

Gojsonnet inside the kapicorp/kapitan:v0.30.0-rc.0 docker image ? #778

Open jperville opened 3 years ago

jperville commented 3 years ago

Describe the bug/feature

The release notes for v0.30.0-rc.0 say that gojsonnet support has been merged, yet the kapicorp/kapitan:v0.30.0-rc.0 docker image does not embed the gojsonnet python package.

To Reproduce

# check for gojsonnet support inside the kapitan source
docker run --rm -ti -w /opt/venv --entrypoint /bin/bash kapicorp/kapitan:v0.30.0-rc.0 -ce 'grep -r gojsonnet .'
# check for the gojsonnet library inside the kapitan virtualenv
docker run --rm -ti -w /opt/venv --entrypoint /bin/bash kapicorp/kapitan:v0.30.0-rc.0 -ce 'find | grep gojsonnet'

Expected behavior

We can see that the current kapitan source in /opt/venv has the support for gojsonnet as documented in the release notes. However the gojsonnet python module is not present in the image (the find command above should return it somewhere in the python3.7 site-packages) resulting in gojsonnet support not being available in the docker image.

If it's a bug (please complete the following information):

Additional context

I tested this rc.0 image to play with the gojsonnet support. I have high hopes that gojsonnet can help reduce significantly the compilation time of my project.

ramaro commented 3 years ago

Hey @jperville at the moment this is still optional support in the sense that gojsonnet will be used if the module is present in the system. So that is why you won't see it installed in the docker image, but if using pip you can install gojsonnet additionally. Maybe going forward it makes sense to have a flag in compile to use gojsonnet instead?

ramaro commented 3 years ago

Another option would be to support gojsonnet and jsonnet types in the compile item configuration.

ramaro commented 3 years ago

@pvanderlinden @simu @janeklb what do you think?

janeklb commented 3 years ago

Could we publish a separate docker image with gojsonnet? Or always include the gojsonnet package but have a cli flag to enable it? Happy to submit a PR for the latter

janeklb commented 3 years ago

Another option would be to support gojsonnet and jsonnet types in the compile item configuration.

I don’t think it makes sense to differentiate the configuration type based on the jsonnet implementation

pvanderlinden commented 3 years ago

I think the main issue with gojsonnet is that it might be to complicated for some to install? With a docker image the user would not have that issue, so maybe it would be good to just have gojsonnet as a default in the docker image? Unless @janeklb thinks it needs more testing in the wild before making it the default.

jperville commented 3 years ago

Thank you everyone for the feedback, looking forward to having a jsonnet-enabled docker image to test.

janeklb commented 3 years ago

Imo we have the following options:

  1. Ship an additional gojsonnet-enabled docker image (eg. kapicorp/kapitan:0.30.0-gojsonnet, or maybe even more official via a new image name kapikorp/kapitan-gojsonnet)
  2. ~Always install gojsonnet (ie, take it out of setup.py's extras section) and allow users to opt in via something like a --gojsonnet cli flag or USE_GO_JSONNET=true envar~ (prob not a good idea cause it would mean building the gojsonnet python package, which requiress go etc.)
  3. Ship kapitan with gojsonnet built in from the start

While i've personally been using the gojsonnet variant for a good while now, I'm not confident that my usage has exercised all the edge-cases that may or may not cause problems under the go implementation of jsonnet. My preference would be to ship it via option 1 or 2 - I don't mind which.

Re users installing it locally (ie. pip install kapitan), the solution there is just to run pip install gojsonnet==<whatever-version-we-use> in the same python environment, and it'll ~automatically work 🎉~ work as long as they have golang installed etc

jperville commented 3 years ago

I tried to install gojsonnet inside the kapitan 0.30.0.rc.0 docker image, not that easy:

docker run --rm -ti -u 0 -w /opt/venv --entrypoint /bin/bash kapicorp/kapitan:v0.30.0-rc.0 -ce '
apt-get update && apt-get install -qy golang-go && export GOPATH=/opt
source bin/activate
pip3 install gojsonnet==0.17.0

Even if I manually install the golang-go package and go get github.com/google/go-jsonnet in the GOPATH, the build of go-jsonnet fails. I think that there should be extra instructions in the Dockerfile to build with go-jsonnet support.

Since go adds a lot of dependencies I vote for option 1 (additional gojsonnet-enabled docker image).

simu commented 3 years ago

Another option would be to support gojsonnet and jsonnet types in the compile item configuration.

I don’t think it makes sense to differentiate the configuration type based on the jsonnet implementation

I agree that it doesn't make sense to differentiate the compile type based on the Jsonnet implementation.

Otherwise we (https://github.com/projectsyn) are not affected by the discussion/decision here, as we're building our own Docker images.

janeklb commented 3 years ago

I've opened up a poc PR (#779) to show how it could work, but we'll need to update the github actions as well as the Dockerfile.ci file to accommodate this.

janeklb commented 3 years ago
docker run --rm -ti -u 0 -w /opt/venv --entrypoint /bin/bash kapicorp/kapitan:v0.30.0-rc.0 -ce '
apt-get update && apt-get install -qy golang-go && export GOPATH=/opt
source bin/activate
pip3 install gojsonnet==0.17.0

@jperville reason the above doesn't work is cause debian stretch only has golang 1.7.x available :(

ramaro commented 3 years ago

@janeklb @simu @jperville I still believe there's value in keeping both jsonnet and gojsonnet. I feel that perhaps the way to go is to either introduce a compile flag (e.g. --use-go-jsonnet) or and env var (e.g. USE_GO_JSONNET=1). #779 is a good idea but the decision to package a specific version at build might be problematic, I think.

wdty?

janeklb commented 3 years ago

I feel that perhaps the way to go is to either introduce a compile flag (e.g. --use-go-jsonnet) or and env var (e.g. USE_GO_JSONNET=1).

For the dockerised version that would make sense; however, imo it would result in a worse user experience for those using the python module directly. Why? In order for that flag to work, gojsonnet must already be installed into your python env, and in order to install gojsonnet you also need golang. This means that kapitan would not be pip-installable on environments where a recent version of golang is not available.

That said, if you're looking to primarily distribute kapitan as a docker image, then that might make sense since you would have full control over what you bundle into the image (though I would selfishly discourage that because kapitan via docker is not my preferred way of compiling locally due to docker volume mount performance).

Another option could be to not list gojsonnet as a python module dependency, and error out if the --use-go-jsonnet flag is passed but gojsonnet is not available. This would mean re-working the current import mechanism; however, it does have the (big) benefit of being explicit rather than implicit.

In terms of what kapitan's "default" jsonnet implementation should be.. maybe start by offering gojsonnet as the new alternative, and then eventually (in X versions?) make the c++ bindings version as the "old/deprecated" option. Ideally, if gojsonnet becomes pip-installable with ease, and gojsonnet has 100% feature parity with jsonnet (which it may already?), the old/deprecated version could actually be completely dropped.

779 is a good idea but the decision to package a specific version at build might be problematic, I think.

Please elaborate... I'm now curious why it might be problematic :)

janeklb commented 3 years ago

To keep it simple for now I would suggest the following as a release:

ramaro commented 3 years ago

@janeklb very valid points! Let's go with that 👍🏼

github-actions[bot] commented 3 months ago

This issue is stale because it has been open for 1 year with no activity. Remove the stale label or comment if this issue is still relevant for you. If not, please close it yourself.