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

Docker: Allow file based declaration of active modules #10036

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

Conversation

Brutus5000
Copy link
Contributor

By default, the file is empty but a user can mount a different file into the container.

In K8s it is unsafe to first run the container and then later on activate modules. This should be done before nodebb is started.
Activating modules for each pod is safe as it is an idempotent operation.

@CLAassistant
Copy link

CLAassistant commented Nov 26, 2021

CLA assistant check
All committers have signed the CLA.

@julianlam
Copy link
Member

If I am reading this correctly, you are specifying that specific plugins be activated when the Dockerfile is run. However, plugin activation states are stored in the database, even though they can be modified via the command line.

So this change does allow you to specify active modules via file, but it would cause side effect like accidental re-activation of a plugin that had been deactivated during the normal running of the instance.

There also shouldn't be any need to repeatedly activate already active plugins, although there's no real harm in doing so.

@Brutus5000
Copy link
Contributor Author

Brutus5000 commented Nov 26, 2021

If I am reading this correctly, you are specifying that specific plugins be activated when the Dockerfile is run.

Yes. They are installed via npm and then activated.

However, plugin activation states are stored in the database, even though they can be modified via the command line.

Yes, this is correct and that's how I do it in my current docker-compose setup. But there I mounted the node_modules folder so I don't lose installed plugins. In a stateless container setup you don't want to mount such a thing. Also you usually don't want to activate or deactivate features at runtime. If you don't want any plugins leave the file empty (which is the default behavior). If you no longer want an addon remove the plugin from the file (and deactivate it via command line or frontend).

So this change does allow you to specify active modules via file, but it would cause side effect like accidental re-activation of a plugin that had been deactivated during the normal running of the instance.

Correct. If a user does not want this feature, he can just leave the file empty which would keep the current behavior.

There also shouldn't be any need to repeatedly activate already active plugins, although there's no real harm in doing so.

Yes, that's the point why I'm doing this. It does no harm, but it makes the container predictable. You specify that you want to use a plugin in the container config and you will get it.

@Brutus5000
Copy link
Contributor Author

Brutus5000 commented Jan 16, 2023

@julianlam What is the status on this? I noticed that #10767 implemented a "similar" feature, however it does not solve the issue of installing the desired module via npm install (my solution does). This makes installing it in Docker and/or Kubernetes quite complicated.

Also: activating the module could be removed as this is possible from the config now. Or I read all active modules from the config but then I need to install jq into the container for parsing.

@oplik0
Copy link
Contributor

oplik0 commented Jan 16, 2023

So as the author of the PR with plugin state in configuration:

  1. I don't think duplicating the state yet again is desired.
  2. Already to handle the overlap between config.json, when there are plugins defined there all attempts at modifying plugin states in other ways than the config will return an appropriate error. While reactivating on container startup makes it "predictable" it does result in some state potentially unexpectedly being lost between startups, so I personally prefer being honest about the plugin settings not really being changed - however this would mean NodeBB would need to actually track this file, which goes back to the duplication
  3. While repeatedly running npm install <package> and ./nodebb activate <plugin> isn't harmful, and it won't happen without the active_modules file, I just wanted to note that there is a time cost - npm install even on an already installed plugin does take a moment and I suspect it might add up fairly quickly.

There is definitely an issue with there not having any way to actually install plugins before container startup. However, NodeBB does have a facility for doing setup before starting for the first time already. And it's even included in the Dockerfile!
So I think a better solution might be to allow setup to install enabled plugins. This might be hidden behind a CLI flag and/or another config option, but generally it'd allow for somehow properly doing this only if SETUP env variable is set, while keeping the existing implementation of the plugin state in the configuration.

@Brutus5000
Copy link
Contributor Author

So I think a better solution might be to allow setup to install enabled plugins.

Yes I think this is actually the core of my problem.

By default, the file is empty but a user can mount a different file into the container.

In K8s it is unsafe to first run the container and then later on activate modules. This should be done before nodebb is started.
Activating modules for each pod is safe as it is an idempotent operation.
@Brutus5000
Copy link
Contributor Author

Brutus5000 commented Feb 5, 2023

So I noticed an issue with both my approach an @oplik0 PR: It ignores the version restriction of a plugin and just grabs the latest version. If I'm not 100% mistaken the NodeBB plugin panel only pulls supported version. 😞

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.

None yet

4 participants