SwissDataScienceCenter / renku-project-template

A repository for default Renku project templates.
https://renkulab.io
4 stars 20 forks source link

build: set renku version at build time #86

Closed rokroskar closed 4 years ago

rokroskar commented 4 years ago

closes #85 closes #76

ableuler commented 4 years ago

Regarding @ableuler's comment, it seems that you need to provide a default argument inside the Dockerfile, otherwise it does not build.

That's what I see as the desired behaviour: the build should fail unless you provide the required build arguments on the command line.

rokroskar commented 4 years ago

I don't really see why it should fail. By removing an explicit version of renku-python from the images, they will be updated much less frequently. So having a default in there will yield a reasonable image most of the time. This is better than failing with some obscure docker error IMO. With a default in there, you can do a simple

docker build -t myimage . 

without any further explanation of advanced docker features.

ableuler commented 4 years ago

But this image might be different than the image built for the same project on Renkulab. I'd find this very confusing.

rokroskar commented 4 years ago

If the default in the .gitlab-ci.yml file and the default in the Dockerfile are the same, it will be the same image 99% of the time.

rokroskar commented 4 years ago

The only reason I included the RENKU_BASE_IMAGE variable in the .gitlab-ci.yml file is so that you can see at a glance what environment you are getting on renkulab.

ableuler commented 4 years ago

Wasn't the reasoning behind setting these args in .gitlab-ci.yml that we would update versions for the user (for example on the click of a UI button) by modifying the versions in .gitlab-ci.yml? Did I misunderstand something there?

pameladelgado commented 4 years ago

The only reason I included the RENKU_BASE_IMAGE variable in the .gitlab-ci.yml file is so that you can see at a glance what environment you are getting on renkulab.

I thought this was also because we want to have another way of easily updating base images as well for when we add some cool feature like mem plugin.

rokroskar commented 4 years ago

Wasn't the reasoning behind setting these args in .gitlab-ci.yml that we would update versions for the user (for example on the click of a UI button) by modifying the versions in .gitlab-ci.yml? Did I misunderstand something there?

Yes, it makes that easier as well.

ableuler commented 4 years ago

But then the versions set as build args in .gitlab-ci.yml and the defaults in the Dockerfile will diverge, which is why I would prefer to not include the same information in two places.

rokroskar commented 4 years ago

yes they will, but see my comment about this above - we will not need to update the default images nearly as often as before. And I would prefer to have the possibility of dead-simple builds without having to run a lap around renku documentation and docker hub image tag lists first.

ableuler commented 4 years ago

Just to be clear: I meant the defaults for both RENKU_BASE_IMAGE and RENKU_VERSION that I would remove from the Dockerfile.

rokroskar commented 4 years ago

For RENKU_VERSION maybe we can just skip the pipx part if it's not set?

rokroskar commented 4 years ago

OK I moved things around so the version of the base image and renku-python is set in the Dockerfile only - this way running docker build will yield identical results everywhere.

rokroskar commented 4 years ago

I'll still need to update the base image versions once https://github.com/SwissDataScienceCenter/renkulab-docker/pull/112 is merged.

emmjab commented 4 years ago

hang on -- i would have expected that we keep the version specified in the gitlab ci, and the variable used in the Dockerfile.

rokroskar commented 4 years ago

why? gitlab-ci is specific to gitlab, while keeping it in the Dockerfile makes it more generic.

rokroskar commented 4 years ago

@emmjab I fixed the reference to github releases and changed it to point to PyPI instead.