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

feat: initial docker implementation #196

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jackstar12
Copy link
Contributor

@jackstar12 jackstar12 commented Sep 5, 2023

This is my initial implementation of #194

It works by choosing the appropiate Dockerfile for the plugin language, building a container and then generating a small script that starts it.

While testing I noticed that many plugins unfortunately dont seem to specify their requirements correctly, e.g.

  • historian missing inotify
  • monitor missing packaging

@netlify
Copy link

netlify bot commented Sep 5, 2023

Deploy Preview for coffee-docs canceled.

Name Link
🔨 Latest commit 9cf8530
🔍 Latest deploy log https://app.netlify.com/sites/coffee-docs/deploys/64f740b5b9921600079aa5f6

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Thanks, I need to look into this in detail but I had a couple of points

  • Why do this inside the coffee manager if the plugin itself can specify how to build it?
  • Why use docker and not something like nix-shell?

As the CI is telling us, we can still work if there is no dockerfile specified, why crash?

@jackstar12
Copy link
Contributor Author

  • Why do this inside the coffee manager if the plugin itself can specify how to build it?

That'd be an additional step for the plugin to consider (providing a build script etc.)

  • Why use docker and not something like nix-shell?

something else could be used, but I suppose docker is the most wide-spread and understood

As the CI is telling us, we can still work if there is no dockerfile specified, why crash?

that shouldn't happen, but I haven't figured out how I could catch the error from the sh! call because it directly returns the error. Maybe sh should be a function?

@vincenzopalazzo
Copy link
Contributor

That shouldn't happen, but I haven't figured out how I could catch the error from the sh! call because it directly returns the error. Maybe sh should be a function?

the macros are already catching the error, if the build fails the error is returned to the user

something else could be used, but I suppose docker is the most wide-spread and understood

Yeah but your code is pulling a new image to build just a single plugin (it should also store the plugin in the local machine), but this is something that you can do already with the following coffee conf

---
plugin:
  name: btcli4j
  version: 0.0.1
  lang: java
  install: |
    docker-compose --build .
  main: .docker-data/btcli4j-gen.sh

Same with nix

plugin:
  name: btcli4j
  version: 0.0.1
  lang: java
  install: |
     nix-shell -p jdk --run make
  main: ./btcli4j-gen.sh

That'd be an additional step for the plugin to consider (providing a build script etc.)

They already need to consider this because currently, they need to tell us where the main file is. And they should do that because otherwise you can not build go plugins or c++ plugins. In addition, do this inside the plugin help the developed to define also additional dependencies that he/she wants (Think about clboss)

I see the code that you write more as a restriction for plugins users, and also I am worried for performance of docker in small device like raspberry pi2/3

However I want this function build it inside the plugin, but I think this should be more a rewrite with a strategy pattern like we are doing with the nurse command https://github.com/coffee-tools/coffee/blob/master/coffee_core/src/nurse/strategy.rs

P.S: Ah the python plugin can not be built in a separate env, otherwise when core lightning run it there is a python missing package, so for dynamic plugins this is a waste of time?

@jackstar12 jackstar12 marked this pull request as draft September 6, 2023 09:32
@jackstar12
Copy link
Contributor Author

Yeah but your code is pulling a new image to build just a single plugin

the pulled image can be reused for subsequent builds of the same language tough. But I see a potential problem where a plugin might depend on a specific python version, but thats a problem thats present right now anyways

However I want this function build it inside the plugin, but I think this should be more a rewrite with a strategy pattern like we are doing with the nurse command https://github.com/coffee-tools/coffee/blob/master/coffee_core/src/nurse/strategy.rs

ill look into that

P.S: Ah the python plugin can not be built in a separate env, otherwise when core lightning run it there is a python missing package, so for dynamic plugins this is a waste of time?

Do you mean the pyln libraries shipped with cln? https://github.com/lightningd/plugins#pythonpath-and-pyln

@vincenzopalazzo
Copy link
Contributor

Do you mean the pyln libraries shipped with cln? https://github.com/lightningd/plugins#pythonpath-and-pyln

I mean if you build a plugin in a separate env how the python code runner by cln know where to look about this dependencies?

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