-
Notifications
You must be signed in to change notification settings - Fork 4
SS Proxy Update #132
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
base: main
Are you sure you want to change the base?
SS Proxy Update #132
Conversation
|
||
# Update sudoers | ||
# Using tee to append to sudoers to handle permissions, and ensuring lines are added only once. | ||
if ! sudo grep -q "www-data ALL=(ALL) NOPASSWD: /usr/sbin/apachectl configtest" /etc/sudoers; then |
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.
Just a nitpick, but everywhere else you've been good to wrap IF conditions with brackets- why not here? also, Probably better to compare the actual return code -ne 0
or something.
required_providers { | ||
aws = { | ||
source = "hashicorp/aws" | ||
version = "~> 5.0" |
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.
Can this be pinned more specifically? I'm just worried about the edge-case of some other terraform code trying to interact with resources created by this choking on a version difference (and yes, that happens, and is why unity-proxy is very specifically pinned)
required_version = ">= 1.0" | ||
|
||
# Local backend for state management | ||
backend "local" { |
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.
why not a S3 backend? As a more fault-tolerant option when our instances evaporate
} | ||
|
||
# Download files from S3 | ||
aws s3 sync s3://${S3_BUCKET}/ /etc/apache2/venues.d/ --exclude "*" --include "*.conf" --delete --quiet |
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.
For this, it doesn't seem like we have a nice failure state- that is, if the configtest
fails, we still have a broken configuration in-place, right?
Could we have something to copy-out the original config, then pull the new config, and copyback on failure?
(something as simple as rsync -ax --delete /etc/apache2/venues.d/ /etc/apache2/venues.d.reload_safe/
and vice versa)
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 have some questions/nitpicks, but overall approve.
Purpose
Proposed Changes
Issues
Testing