Move nginx config files to subdirectory#1379
Conversation
* tidy up the repo a little * prepare for potential changes to the config in e.g. getodk#1376
There was a problem hiding this comment.
Given that the command in the Procfile is still -p "$PWD", do any file paths need to change in main.nginx.conf? For example, for something like:
include ./common-headers.nginx.conf;Isn't ./ still the top-level directory of the repository? I'm wondering whether that line needs to change to:
include ./nginx-conf/common-headers.nginx.conf;Or are include paths relative to the config file rather than the -p path?
Would it help anything to change the -p path to "$PWD/nginx-conf"?
There was a problem hiding this comment.
Is there any chance you could give this branch a quick try? Given the motivation for this PR was #1376, which was motivated by not being able to run nginx -p in the first place...
There was a problem hiding this comment.
@matthew-white did you get a chance to test this?
There was a problem hiding this comment.
Just tried it, sorry for the delay! It's working. 🥳 I guess that means that some paths in the config file are relative to the -c path, not the -p path. That's good to know.
When I run npm run dev and load Frontend locally, I see:
- A new nginx.pid file is created. That indicates that the
piddirective is working. - New entries are added to nginx-access.log. That indicates that the
access_logdirective is working. - The response headers in common-headers.nginx.conf are returned. That indicates that the
includedirective is working.
What I didn't check:
alias ./dist/blank.html;for /-/single/check-submitted. Conceptually, it makes sense to me thataliaswould be relative to the-ppath. We can always change it if that turns out to be wrong.client_body_temp_pathandproxy_temp_path. Again, it makes sense to me that those would be relative to the-ppath.pidandaccess_logare both still working and similarly write to files in the .nginx directory.npm run dev:builddidn't work, so I wasn't able to check whetherroot ./dist;works as intended. However, it looks likenpm run dev:buildis broken on themasterbranch as well. I can file an issue about that. I almost always usenpm run dev, which is probably why I hadn't noticed this already. I don't think this issue needs to block this PR from being merged.
There was a problem hiding this comment.
it looks like
npm run dev:buildis broken on themasterbranch as well. I can file an issue about that.
Filed at getodk/central#1606.
There was a problem hiding this comment.
... and now I've closed the issue after realizing that npm run dev:build is working after all. I've confirmed that it works on the current master branch, which means that it plays well with the changes from this PR. It also indicates that root ./dist; is working correctly. That implies that root operates relative to the -p path, which makes sense.
What has been done to verify that this works as intended?
Nothing - I can't currently start nginx locally from the
Procfile.Why is this the best possible solution? Were any other approaches considered?
Discussed at #1376 (comment)
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
No change - dev only.
Does this change require updates to user documentation? If so, please file an issue here and include the link below.
No - the change is dev only.
Before submitting this PR, please make sure you have:
npm run testandnpm run lintand confirmed all checks still pass OR confirm CircleCI build passes