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

Adding Kosmtik plugins to Docker #2679

Merged
merged 1 commit into from
Jul 9, 2017

Conversation

kocio-pl
Copy link
Collaborator

@kocio-pl kocio-pl commented Jul 8, 2017

This code adds some plugins to make Kosmtik more usable.

I don't see why .config directory was created, because .kosmtik-config.yml seems to be used outside it, so I removed that line.

Copy link
Contributor

@nebulon42 nebulon42 left a comment

Choose a reason for hiding this comment

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

Reasonable extension, but to avoid layer overhead please group commands together.

Dockerfile Outdated
RUN npm install -g kosmtik

WORKDIR /usr/lib/node_modules/kosmtik/
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should not use WORKDIR here. Why do you need it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Something seems to be wrong with plugins npm installation. When Kosmtik is installed globally and plugins are installed with "i" - as advised on their npm pages - plugins are installed in the local (working) directory and are not visible to the program. I was trying to find a way to install them in /usr/lib/node_modules/kosmtik/node_modules/ through npm options (like --prefix), but failed.

I guess npm installation of plugins needs to be examined by the Kosmtik team to work out of the box.

Copy link
Contributor

@nebulon42 nebulon42 Jul 8, 2017

Choose a reason for hiding this comment

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

I have to admit I never thought about that. Maybe try to install the plugins globally too? With npm install -g plugin? I'll have to try it. Meanwhile, could you please open a issue about that at Kosmtik too?

Copy link
Contributor

@nebulon42 nebulon42 Jul 8, 2017

Choose a reason for hiding this comment

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

At the Kosmtik README it says that plugins should be installed with kosmtik plugin --install pluginname. But then we have problems with .kosmtik-config.yml. This problem exists also with your approach. The reason why I moved the config file out of the container was for it to be customized locally. But at build time of the container we cannot symc it to the host. That is why you tried to copy it from tmp to the WORKDIR. But this overwrites any existing file, which is undesirable.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only solution I see is to add .kosmtik-config.yml to version control with the right plugin entries in it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is also not the perfect solution too. When the kosmtik-place-search plugin will be working, I would like to add it to repo and then what? If the user changed the file, we should parse it and add this plugin name in the proper place (maybe other than just the end of the file). It also means that every change a user makes to the config file will probably alarm git and make updating code harder. It's not a good idea to treat configuration like the code.

That's why I've tried a brute force approach - at least configuration file will be in sync with installed plugins. So I guess I will stick to that way, but I will just use the test - if the config file is already here, user is on his own, since we don't know what it looks like. OK, we can even test if this is original file we left here before (with MD5 hash), so we can update it automatically, but this might be overengineering.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've just checked that without WORKDIR kosmtik plugins --install pluginname uses filesystem root (/) and it's not working too, of course. Maybe Kosmtik should have some different plugins discovery/configuration method, because currently global npm installation doesn't work with plugins - there are two different ways to do it, for example:

node index.js plugins --install kosmtik-place-search

and

npm i kosmtik-place-search

(on https://www.npmjs.com/package/kosmtik-place-search) and both are broken.

Copy link
Contributor

Choose a reason for hiding this comment

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

For me kosmtik plugins --install pluginname worked. The only problem was with the config file.

Dockerfile Outdated
RUN npm install -g kosmtik

WORKDIR /usr/lib/node_modules/kosmtik/
RUN npm i kosmtik-overpass-layer
Copy link
Contributor

Choose a reason for hiding this comment

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

Every RUN creates another layer (see https://stackoverflow.com/questions/31222377/what-are-docker-image-layers). Common practice is to group commands together. So why not writing RUN npm i kosmtik-fetch-remote kosmtik-overlay ....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, you're right! I was testing plugins, so it was just easier to turn them on and off.

Dockerfile Outdated
RUN npm i kosmtik-mapnik-reference
RUN npm i kosmtik-geojson-overlay

RUN echo "plugins:" >> /tmp/.kosmtik-config.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

Here applies the same about layers. You could either sacrifice readability a bit and move everything you have to echo in one string with \n or you could also group commands together with && thus writing:
RUN echo "plugins:" >> /tmp/.kosmtik-config.yml && echo " ..." ...

@@ -32,7 +32,7 @@ import)
kosmtik)
python scripts/get-shapefiles.py -n

mkdir -p .config
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a leftover, but I'm not sure now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely a leftover.

@kocio-pl
Copy link
Collaborator Author

kocio-pl commented Jul 8, 2017

Code updated, it's shorter now, since this plugin installation method creates configuration file itself.

However (re: #2679 (comment)) we already have a configuration/code mix problem with .env file. When I've changed it, git complains about that change. It should be ignored by git probably.

@nebulon42
Copy link
Contributor

Maybe another solution for the problems with both .env and .kosmtik-config.yml. For .env:

  • on startup of import we check if the file exists locally
  • if yes, we just use it
  • if not we create it with default values
  • it is added to .gitignore

For .kosmtik-config.yml:

  • on install of kosmtik we create it with default values (plugins)
  • on startup of kosmtik we check if the file exists locally
  • if yes, we just use it
  • if not we sync it out to the host
  • it is still part of .gitignore

With this approach we should get reasonable default values the first time and let the user customize it. Only downside with .kosmtik-config.yml would be that if there are changes software-wise the config file would have to be updated by the user or deleted first before starting kosmtik container.

Dockerfile Outdated
--install kosmtik-mapnik-reference \
--install kosmtik-geojson-overlay

RUN cp /root/.config/kosmtik.yml /tmp/.kosmtik-config.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

If we could tie this cp with && to the kosmtik plugin install I'm happy. :)

@nebulon42
Copy link
Contributor

nebulon42 commented Jul 9, 2017

I have verified that it indeed does not work without setting the workdir. We have to tackle that from the Kosmtik side. And I saw that you already do for the Kosmtik config what I suggested. So there would only be one minor comment left, otherwise I'm fine with it.

The thing with .env can be done in another PR.

@kocio-pl kocio-pl merged commit 91fd10a into gravitystorm:master Jul 9, 2017
@kocio-pl kocio-pl deleted the kosmtik-plugins branch July 9, 2017 14:13
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