-
Notifications
You must be signed in to change notification settings - Fork 0
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
Run without docker #649
Run without docker #649
Conversation
Update dev stacks to silence warnings
Effectively runs the same command as docker entrypoint
Add python version and nvmrc
Easier to test this way
For launch template Add secrets spec; update install_secrets For more information about how this secrets spec works, see openstax/aws-ruby repository Turns out top_key is not optional in secrets spec That README in openstax/aws-ruby should really be updated When the secrets factory is being created, `.to_sym` is called on the top_key That means it cannot be nil Then top_key is used as a key for the secrets spec, even if it is an empty string Consequently, top_key is required
We have had problems with poetry lock file compatibility in the past This file will be read by the ansible playbook to determine which version of poetry to install
This will need to work differently in the future if it's ever revived Leaving the scripts for now as they might still be useful
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.
I reviewed the code only, which looks good to me. I did not attempt to run it or anything.
errorlog = "-" | ||
errorlog = error_log_file | ||
accesslog = access_log_file | ||
syslog = syslog_conf is not None |
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.
Very minor thing, could move this syslog =
up a bit and use it in the check if syslog_conf ...
so the condition is always the same
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.
I see your point, but I like keeping all the gunicorn settings together like this. Otherwise it can be difficult to tell which variables are used for preparing the settings (and therefore have no meaning to gunicorn) vs which ones hold the actual settings values that are used by gunicorn. Right now all the variables under the comment (except for log_data
) are the ones that gunicorn actually uses.
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.
Another thing, from reviewing the deployment PR, it seems you have the fluent_bit config in there so you shouldn't really need syslog to forward anything. If I remember right, I had fluent_bit forward the systemd and syslog logs to CloudWatch. The way it is now, you have to specify the syslog identifiers though, because I think otherwise you get too much stuff.
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.
Unless there is some kind of hidden configuration happening that I am unaware of, the syslog part of this config is completely unused.
I updated this config before I knew about fluent_bit, etc. I wanted to try to avoid a situation where I would need to coordinate changes in multiple repositories and rebuild the ami while troubleshooting issues, so I added support for all the different configurations I could think of. Maybe I should revert this change to avoid confusion.
This version should be idempotent (hopefully)
((expression)) The expression is evaluated according to the rules described below under ARITHMETIC EVALUATION. If the value of the expres‐ sion is non-zero, the return status is 0; otherwise the return status is 1. This is exactly equivalent to let "expression". Cool, that seems very useful and not at all confusing/error-prone when combined with -e.
Changes for new deployment process.
start-bare-metal
command (easy entrypoint)install_app
script to install dependencies on startup (runs in launch template)install_secrets
script to install secrets into the .env file on startup (currently runs in launch template, could potentially be part of start-bare-metal command instead)secrets_spec.yml
(for aws-ruby secrets specification)start-bare-metal
)Environment variables are passed into the start script directly in
start-bare-metal
. I did it this way because it's similar to how it was already done. It is still possible to read the environment variables withsudo strings /proc/<PID>/environ
(or by reading the .env file).Things that could be done, but didn't seem important: