feat: [Improvement] Adds FORUM_ENV filter to manage forum environment vars

[xitij2000]

The forum service is entirirely configured using environemnt variables, and there are a number of environment variables that are not configurable via tutor currently. This change adds patches in those places allowing additional configuration variables to be passed.

[regisb]

I understand the need to configure the forum via environment variables. I really wish there was a better way to override environment variables than to implement multiple patches, but I thought about it long and hard and did not find a very elegant solution. This PR shows that there is already quite a bit of duplication in the forum templates.

EDIT: Scratch that, I have a better idea (see below).

[regisb]

I think I got a better idea:

FORUM_ENV = tutor.core.hooks.Filter()
FORUM_ENV.add_items([
    ("SEARCH_SERVER", "{{ ELASTICSEARCH_SCHEME }}://{{ ELASTICSEARCH_HOST }}:{{ ELASTICSEARCH_PORT }}"),
    ...
])

@tutor.hooks.Filters.ENV_PATCHES.add()
def _forum_patches(patches):
    k8s_env_patch = ""
    local_env_patch = ""
    for key, value in FORUM_ENV.iterate():
        k8s_env_patch += f"- name: {key}\n  value: {value}\n"
        local_env_patch += f"{key}: {value}\n"
    patches += [("forum-k8s-env", k8s_env_patch), ("forum-local-env", local_env_patch)]
    return patches

And then in the k8s templates:

env:
    {{ patch("forum-k8s-env")|indent(12) }}

(and similar in the docker-compose templates)

With this approach, users would only have to add items to the FORUM_ENV filter to define new environment values. Moreover, this would remove a lot of duplication between forum templates.

Please tell me whether this makes sense, I understand it might be a little confusing.

[xitij2000]

I think I got a better idea:

FORUM_ENV = tutor.core.hooks.Filter()
FORUM_ENV.add_items([
    ("SEARCH_SERVER", "{{ ELASTICSEARCH_SCHEME }}://{{ ELASTICSEARCH_HOST }}:{{ ELASTICSEARCH_PORT }}"),
    ...
])

@tutor.hooks.Filters.ENV_PATCHES.add()
def _forum_patches(patches):
    k8s_env_patch = ""
    local_env_patch = ""
    for key, value in FORUM_ENV.iterate():
        k8s_env_patch += f"- name: {key}\n  value: {value}\n"
        local_env_patch += f"{key}: {value}\n"
    patches += [("forum-k8s-env", k8s_env_patch), ("forum-local-env", local_env_patch)]
    return patches

And then in the k8s templates:

env:
    {{ patch("forum-k8s-env")|indent(12) }}

(and similar in the docker-compose templates)

With this approach, users would only have to add items to the FORUM_ENV filter to define new environment values. Moreover, this would remove a lot of duplication between forum templates.

Please tell me whether this makes sense, I understand it might be a little confusing.

I think this makes a lot more sense! I was just reluctant to add new config to the main project.
Here, instead of using a patch though, wouldn't it be better to just directly iterate over FORUM_ENV in the templates?

[xitij2000]

Just realised what I proposed won't work since the values won't be rendered. So I've implemented your approach.

[xitij2000]

I generated the config with and without this change and here is the diff:

So nothing that should break anything.

[gabor-boros]

👍 @xitij2000

[regisb]

I only have a couple minor comments, otherwise I think this is top notch. @ghassanmas what do you think?

[ghassanmas]

This looks interesting change, I have quickly tested and provided a comment above... I will keep an eye on this and expect me to test it locally with docker/compose. But I wouldn't test it with k8s as it's still a black box to me.

A curious quesiotn I have; What would happen if opreator would use the k8s/local patch directly? like do we expect them to do so? I suppose it might be the case if I have different configruation between local and k8s

Another thing that also make the situation more obscure to me is that it might be the case that we run k8s locally hence

• How to run Open edX on a local Kubernetes cluster? openedx/wg-devops#4

But this I guess then something related to tutor core naming as always naming might not reflect the meaning :-)

[xitij2000]

This looks interesting change, I have quickly tested and provided a comment above... I will keep an eye on this and expect me to test it locally with docker/compose. But I wouldn't test it with k8s as it's still a black box to me.

A curious quesiotn I have; What would happen if opreator would use the k8s/local patch directly? like do we expect them to do so? I suppose it might be the case if I have different configruation between local and k8s

Another thing that also make the situation more obscure to me is that it might be the case that we run k8s locally hence

* [How to run Open edX on a local Kubernetes cluster? openedx/wg-devops#4](https://github.com/openedx/wg-devops/issues/4)

But this I guess then something related to tutor core naming as always naming might not reflect the meaning :-)

I can't think for a good reason right now for someone to want different environment variables for k8s vs local. However, they can just use the patches in that case as you mentioned.

I imagine people will generally only deploy an instance using local or k8s and not both from a single config, so if they wanted they can just copy over the config, change the required variables/plugins/settings and deploy to local and k8s from different configs.

I think running k8s on your local computer is a kind of an edge case! I've tested tutor again minikube and I currently run a k3s cluster on a secondary mini-pc exactly for these kinds of tests. I've had success so far, but it definitely takes some effort setting up.

[ghassanmas]

I have tested this locally and it worked as expected, the services init, craeted a thread..etc. The only thing I can think of, is just ot update the readme, and second regarding the order/priority of the patches, my thinking is that we have know three pathces that can override each other:

1. The forum built-in patch that is filled from tutor config
2. The forum env base
3. The local/k8s variants

My understanding and I think that the logical is tha should be loaded in that order, but I haven't tested that yet, looking through the code I see that 2 and 3 has priority high, so I think we should be safe... unless we get into a weired edge case if opreator are using both 2 and 3 and 3 get loaded before 2

[xitij2000]

I have tested this locally and it worked as expected, the services init, craeted a thread..etc. The only thing I can think of, is just ot update the readme, and second regarding the order/priority of the patches, my thinking is that we have know three pathces that can override each other:

1. The forum built-in patch _that is filled_ from tutor config

2. The forum env base

3. The local/k8s variants

My understanding and I think that the logical is tha should be loaded in that order, but I haven't tested that yet, looking through the code I see that 2 and 3 has priority high, so I think we should be safe... unless we get into a weired edge case if opreator are using both 2 and 3 and 3 get loaded before 2

I've updated the readme file with some info about how this hook can be used.

As for the priority. I think that's definitely a potential issue. I'm not sure how to fully avoid it, and perhaps it can't be avoided in all cases. Do you think it's worth warning about this in the readme? I've run into such edge cases in the past and it was hard to diagnose, but perhaps also hard to list all such edge cases.

[regisb]

LGTM! Just a minor formatting issue

[xitij2000]

LGTM! Just a minor formatting issue

@regisb Just checking if this is OK to merge now.

[regisb]

LGTM! As the maintainer of this repo, I'll let @ghassanmas take care of squashing and merging.

[ghassanmas]

Apologies for late respond, I has been recently...
I have teseted this, it worked with no issue with tutor local.

Regarding this:

As for the priority. I think that's definitely a potential issue. I'm not sure how to fully avoid it, and perhaps it can't be avoided in all cases. Do you think it's worth warning about this in the readme? I've run into such edge cases in the past and it was hard to diagnose, but perhaps also hard to list all such edge cases.

I think the way to avoid this, is set the priority FORUM_ENV, hight + 1, this would ensure that other spesfic patches (k8s/local ) has higher priority, given high is just a number.

I haven't tested that though, it's based on my understanding how priority works in tutor, I will give it a look later and update the code accordingly.

Thank you for your contribution, it's much apperciated 🎉 .