-
-
Notifications
You must be signed in to change notification settings - Fork 244
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
base: main
Are you sure you want to change the base?
Conversation
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 |
scripts/generate-compose.sh
Outdated
fi; | ||
|
||
|
||
echo "$compose" > "docker-compose.yml" |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
Need to add tests, linter and a workflow. |
This would still require users to run a script to generate the secrets, right? We can update the .env file in |
We should use |
Got it. We'll be generating a |
def write_env(env): | ||
env_file = '' | ||
for key in env: | ||
env_file = f'{env_file}{key}={env[key]}\n' |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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.
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 :) |
No description provided.