Skip to content

Support portainer#22

Closed
zastrixarundell wants to merge 11 commits into
Vencord:mainfrom
zastrixarundell:support/portainer
Closed

Support portainer#22
zastrixarundell wants to merge 11 commits into
Vencord:mainfrom
zastrixarundell:support/portainer

Conversation

@zastrixarundell
Copy link
Copy Markdown

@zastrixarundell zastrixarundell commented Nov 26, 2023

This PR basically just allows for the repo to be used in portainer.

Explained functionality: Create docker compose file specific for portainer. This file needs to be setup in the repository, or the users need to create their own forks of this project and maintain it. If an official image were published on an OCI repository there would be no need for this PR.

Notes:

  • 8080 is used in healthchecks so I've hardcoded the internal port to 8080, $PORT now affects only the host port.

Remove the hard requirement for the `.env` file inside the repository. This is to be updated so that portainer can use this repository.
Add environment variable support inside the docker-compose.yml file. This allow for portainer to setup variables.
Setup the ports to be a list instead of the current map design. This is a hard requirement by portainer.
Healthcheck for the container requires to have the port `8080` setup. This commit now changes the internal port to be 8080 while the public port is defined by the env file.
Whoops! `REDIS_URI` was missing! This fixes the issue!
This hostname field is probably not required, that's why it's being removed.
Add the PROXY_HEADER env variable back into the docker-compose.yml file
@zastrixarundell
Copy link
Copy Markdown
Author

REDIS_URI can be setup in the example file to (and the docker-compose.yml file) to be already set at redis:6379, no?

@D3SOX
Copy link
Copy Markdown

D3SOX commented Nov 26, 2023

I think it's cleaner for Portainer setups to have it as

env_file:
  - stack.env

instead of duplicating a bunch of variables.
Then one should add those environment variables inside Portainer under the compose editor (it uses stack.env as file name)
The easiest way for that is to enable the advanced editor and just paste the content of .env.example

@zastrixarundell
Copy link
Copy Markdown
Author

I mean that is a solution as well for it. But I'm not sure if it'll work well for portainer as then ports aren't defined and network isn't set as host, so this might be a better solution?

Comment thread docker-compose.yml Outdated
@D3SOX
Copy link
Copy Markdown

D3SOX commented Nov 26, 2023

image
Like this

(Just using Web editor for showcasing it here, this way it wouldn't work as it can't build the image but when you use it as a git repository stack it should work)

@zastrixarundell
Copy link
Copy Markdown
Author

zastrixarundell commented Nov 26, 2023

Yeah yeah we could add stack.env into the docker-compose.yml file and make it the default one. Then the documentation should be updated, but I'm also not really up for doing that. It would break the current deployments for other users as stack.env wouldn't be present.

This way with duplicating the variables in compose file:

  • The variables have their own context in the file so you don't have to look at other files
  • A bit more universal as it's not putting any hard requirements

I would personally go with my implementation over stack.env but it's ultimately for the owner of the repo to decide. I'd really not like to want to have my own fork and maintain it so I can use vencloud...

Edit:

I would be okay if there's a way to set a soft dependency for the env file but I'm not sure if that's a thing.

@D3SOX
Copy link
Copy Markdown

D3SOX commented Nov 26, 2023

Having a unified docker compose file for both standalone docker compose installations and Portainer is generally a bad idea imo.

@zastrixarundell
Copy link
Copy Markdown
Author

zastrixarundell commented Nov 26, 2023

Oh! I didn't think of that honestly! That's a good idea! I'll go make a docker compose file specifically for portainer without modifying the old docker-compose.yml, I forgot it doesn't need to be exactly docker-compose.yml and that the end-user can specify which compose file should be used.

Revert docker-compose.yml to be the same as the actual branch in the decision of making a compose file specifically for portainer.
Add dockerfile which is only to be used for portainer.
Hardcode `REDIS_URI` into the docker compose file for portainer.
Hardcode `HOST` to `0.0.0.0` in the portainer compose file.
@zastrixarundell
Copy link
Copy Markdown
Author

Just as an FYI, this PR isn't ready as-is. The documentation for portainer needs to be added. I'll write the documentation if the PR becomes approved.

@zastrixarundell
Copy link
Copy Markdown
Author

Hm maybe default values should be put in these lines:

HOST = os.Getenv("HOST")
PORT = os.Getenv("PORT")

Let's say 8080 for PORT and 127.0.0.1 for HOST.

With this function:

func getEnv(key, fallback string) string {
    if value, ok := os.LookupEnv(key); ok {
        return value
    }
    return fallback
}

Because the healthcheck is working on port 8080:

HEALTHCHECK --interval=15s --timeout=3s CMD curl -f http://localhost:8080/v1/ || exit 1

Then there would be no reason to define the host and port in the docker-compose.yml.

@Vendicated
Copy link
Copy Markdown
Member

Vendicated commented Jun 23, 2024

can you explain why this is necessary? it seems just the same as the normal docker-compose file, other than renaming .env to stack.env and adding

environment:
    PORT: 8080
    HOST: 0.0.0.0
    REDIS_URI: redis:6379
ports:
    - ${PORT}:8080/tcp

why are these necessary? the three environment variables are already specified in .env and the port mapping via the override. Why the rename to stack.env?

@zastrixarundell
Copy link
Copy Markdown
Author

zastrixarundell commented Jun 23, 2024 via email

@Vendicated
Copy link
Copy Markdown
Member

well thank you for the contribution, but i don't think there is any interest from our side to merge this. we don't know how portainer works and it seems unreasonable to have two almost duplicated configs, especially considering it seems to me as if this config force-overrides some values specified in .env, which is poor user experience. at this time, we also have no plans to publish to any container hub; it seems kinda pointless, considering it's just one simple git clone

it would probably be best for you to just keep this as your personal fork!

@lewisakura lewisakura closed this Jun 26, 2024
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.

4 participants