Issue 312 fix aggregate config #318
Conversation
| } | ||
|
|
||
| if [ ${HELP} = YES ]; then | ||
| showHelp | ||
| exit 0 | ||
| fi | ||
|
|
||
| if [ ${NET_MODE} != "nat" ] && [ ${NET_MODE} != "bridge" ]; then |
There was a problem hiding this comment.
You'll get a unary operator expected error if you don't have a ${NET_MODE} set. The error will happen on line 66, 73, 111, 116.
Related, you might want to run the script through https://www.shellcheck.net to check for other issues.
| @@ -1,168 +1,21 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
I typically like keep the stock file and making the necessary changes. I think it makes for easier diffs with the stock file on a default install and the links to the docs are nice too.
But I'm guessing you removed this to make the grepping easier?
There was a problem hiding this comment.
That's exactly right. I'll add the stock file as well with a .original prefix, just in case
| exit 1 | ||
| fi | ||
|
|
||
| if [ ${NET_MODE} == "bridge" ] && ([ -z ${HTTP_PORT} ] || [ -z ${HTTPS_PORT} ]); then |
There was a problem hiding this comment.
Maybe I'm confused, but this warning is also valid for NAT mode, no?
There was a problem hiding this comment.
Not really, but that's mainly because there isn't clear criteria of which args are required which aren't.
I'd propose this:
- If
--http-portis set, then--https-portis required, and vice versa. - If
--http-portand--https-portsare defined, then--net-modeis required.
(FQDN is always optional)
|
Pretty sure this will require a change in the docs. Can you update the PR description? |
|
Overall, this does what it says on the tin. Just some minor tweaks then it can be merged! |
- At least an arg must be set - http-port and https-port must be set together - net-mode must be set along with http-port and https-port - net-mode must be one of 'nat' or 'bridge'
|
OK! Added issue in docs repo and applied your suggestions, @yanokwa. |
Closes #312
This PR improves the
aggregate-configtool that we ship in the Aggregate VM:--net-modeargument that will accept eithernatorbridgevalues to adapt Tomcat's and Aggregate's configuration to the current VM network device mode.This PR also simplifies the shell command to get the IP addresses on the VM for the MOTD.
What has been done to verify that this works as intended?
Run the commands in the VM, having changed back and forth the network device. Verified that Aggregate was reachable in both nat and bridge scenarios.
Why is this the best possible solution? Were any other approaches considered?
No other approaches considered for this change.
Are there any risks to merging this code? If so, what are they?
Nope.
Do we need any specific form for testing your changes? If so, please attach one
No.
Does this change require updates to documentation? If so, please file an issue at https://github.com/opendatakit/docs/issues/new and include the link below.
getodk/docs#866