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

feature: Added generate-compose script. #295

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

Conversation

sundargs2000
Copy link

No description provided.

@sundargs2000 sundargs2000 marked this pull request as draft April 30, 2021 13:57
@sundargs2000
Copy link
Author

sundargs2000 commented Apr 30, 2021

Hey @timabbott. This was what I had in mind. This is very rudimentary and might need more logic and error handling. I ended up creating .config where you can set the mandatory settings and any other passwords if they want to (we can change it to command line arguments).

fi;


echo "$compose" > "docker-compose.yml"
Copy link
Member

Choose a reason for hiding this comment

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

We should run shellcheck on this, and also set it up as a linter for this repository, as it's a super useful tool for avoiding escaping/etc. bugs when doing shell scripting.

(It's already setup as a zulip/zulip linter).

Copy link
Member

Choose a reason for hiding this comment

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

That, or we should just write this in Python and give it a test suite; I think that might be a happier approach long-term if we end up adding more features to this.

Copy link
Author

Choose a reason for hiding this comment

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

Yup. It would add the expectation the users have python installed but in the long term would be the better option.

Copy link
Author

Choose a reason for hiding this comment

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

Shall I go ahead and change it to Python?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, go for it.

.config Outdated
MEMCACHED_PASSWORD='REPLACE_WITH_SECURE_MEMCACHED_PASSWORD'
RABBITMQ_DEFAULT_PASS='REPLACE_WITH_SECURE_RABBITMQ_PASSWORD'
REDIS_PASSWORD='REPLACE_WITH_SECURE_REDIS_PASSWORD'
SECRETS_secret_key='REPLACE_WITH_SECURE_SECRET_KEY'
Copy link
Member

@timabbott timabbott Apr 30, 2021

Choose a reason for hiding this comment

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

What do you think about changing this to a .ini format, like /etc/zulip/zulip.conf? We can use Python's built-in ConfigParser (or crudini from shell scripts).

I guess the main downside with something like that is that we'll need to require dependencies beyond bash to run this, but I think Python standard library is safe enough.

Copy link
Author

Choose a reason for hiding this comment

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

Seems like the cleaner option whether we go with bash or python.

@flavio-prado
Copy link
Contributor

flavio-prado commented May 1, 2021

Why not have just a .env.example file with all possible configurations and add in the docker-compose.yml the following line

env_file:
      - .env

Only thing that would be needed for the user is copy the example file and change for it's needs without any extra installation.

@sundargs2000
Copy link
Author

Need to add tests, linter and a workflow.

@sundargs2000
Copy link
Author

Why not have just a .env.example file with all possible configurations and add in the docker-compose.yml the following line

env_file:
      - .env

Only thing that would be needed for the user is copy the example file and change for it's needs without any extra installation.

This would still require users to run a script to generate the secrets, right? We can update the .env file in generate-secrets script, could be a cleaner way to do things but issue would be users seeing a ready docker-compose.yml, they might do docker-compose up and face issues with the server, whereas in the current way they won't have a docker-compose.yml initially until they run the generate-secrets. Thoughts @timabbott @flavio-prado?

@andersk
Copy link
Member

andersk commented May 2, 2021

We should use env_file. There’s no concern about a missing env_file causing random server issues; docker-compose up will appropriately refuse to start if an env_file is missing.

@sundargs2000
Copy link
Author

Got it. We'll be generating a .env from .env.example.

def write_env(env):
env_file = ''
for key in env:
env_file = f'{env_file}{key}={env[key]}\n'
Copy link
Member

Choose a reason for hiding this comment

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

This is probably cleaner written using a more natural append code; I found this hard to read.

set_if_expected(env, 'POSTGRES_PASSWORD', 'REPLACE_WITH_SECURE_POSTGRES_PASSWORD', secrets.token_hex(32))
set_if_expected(env, 'MEMCACHED_PASSWORD', 'REPLACE_WITH_SECURE_MEMCACHED_PASSWORD', secrets.token_hex(32))
set_if_expected(env, 'RABBITMQ_PASSWORD', 'REPLACE_WITH_SECURE_RABBITMQ_PASSWORD', secrets.token_hex(32))
set_if_expected(env, 'REDIS_PASSWORD', 'REPLACE_WITH_SECURE_REDIS_PASSWORD', secrets.token_hex(32))
Copy link
Member

Choose a reason for hiding this comment

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

Do these entropy amounts match what's used in generate_secrets.py? If so, we should have a comment saying so.

@klardotsh
Copy link
Member

Looks like this has been sitting for about a year and a half; unless @sundargs2000 or @timabbott want to pick this back up, I might propose closing it as stale at this point, or I can see about adopting it to take over the finish line. Either way, we should probably move to get it out of the open PRs list through some means :)

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.

5 participants