Update variable template placeholder values #86
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
slifty
commented
Jan 20, 2021
| @@ -189,7 +189,7 @@ In order to get the base system up and running, you need to use the install | |||
| from ansible instructions above for the following: | |||
|
|
|||
| * base/mwlib | |||
| * base/torque | |||
| * base/torquedata | |||
There was a problem hiding this comment.
This is unrelated to the PR and snuck in (I can separate it out if you would prefer) -- just a broken reference I noticed when following instructions!
There was a problem hiding this comment.
Ah, no problem. I agree in general with the principle of keeping unrelated changes separate, but let's not slow this change down by overly-eagerly applying that principle here.
slifty
force-pushed
the
85-template-envsubst
branch
3 times, most recently
from
57cb807
to
945b0a5
Compare
kfogel
approved these changes
Jan 21, 2021
|
|
||
| # The port the flask server listens on | ||
| torquedata_port: 5000 | ||
| torquedata_port: ${TORQUEDATA_PORT} |
There was a problem hiding this comment.
Do you want an
# e.g. 5000
here, the way you did with "# e.g. /var/www/html" for HTML_DIRECTORY above?
The base ansible scripts have variable templates for local setup. The templates allow the user to specify various settings. This commit changes the way the placeholder values are represented in these templates, specifically to allow scripts to leverage envsubst in certain circumstances (e.g. when using vagrant to set up a virtual machine for local development) Issue #85
This continues the change where we replace template placeholder values in ansible variable templates with a placeholder format that can work with `envsubst` to insert environment variables. Since this is just a modification to the template placeholders this should have no impact on installation steps (as those values would be replaced by hand anyway). All this does is make it possible for a script to populate a template more easily. Issue #85
Now that our template files support envsubst, lets let the good people know!
slifty
force-pushed
the
85-template-envsubst
branch
from
444a0d2
to
b567073
Compare
|
I got @frankduncan's greenlight offline so I'm gonna merge this in! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our ansible scripts all have variable templates for local installs, and part of running the scripts is populating the templates with real values. This PR replaces the placeholders in those tempaltes so that it becomes possible to use envsubst.
This change should have no impact on any deployed site (since we are just modifying placeholders in templates), but also this shouldn't even change deployment practices since these placeholders can be just as easily populated manually as the other format.