Repository navigation
Fix postfix_set_hostname and get_public_ip functions - #265
Conversation
postfix_set_hostname:
- Fix reversed logic for warning when both POSTFIX_myhostname and
AUTOSET_HOSTNAME are set
- Change condition from -z (empty) to -n (not empty) to properly detect
when both are set
- Fix variable case sensitivity: $IP -> $ip for reverse DNS lookup
- Improve control flow with proper if/else structure
get_public_ip:
- Add missing URL parameter to curl command ("$service")
- Fix array reference in debug output: ${services} -> ${services[0]}
- Redirect logging messages to stderr so only IP is output to stdout
- Add echo "$ip" to return detected IP value
- Add return 0 on successful IP detection to exit function properly
These fixes enable AUTOSET_HOSTNAME=1 to work correctly, auto-detecting
the public IP
and setting the Postfix hostname via reverse DNS lookup.
|
Cool, thank you for the commit! Before merging, please see my comments in the code, though. |
| if [[ -n "${AUTOSET_HOSTNAME_SERVICES}" ]]; then | ||
| services=("${AUTOSET_HOSTNAME_SERVICES}") | ||
| notice "Using user defined ${emphasis}AUTOSET_HOSTNAME_SERVICES${reset}=${emphasis}${AUTOSET_HOSTNAME_SERVICES}${reset} for IP detection" | ||
| notice "Using user defined ${emphasis}AUTOSET_HOSTNAME_SERVICES${reset}=${emphasis}${AUTOSET_HOSTNAME_SERVICES}${reset} for IP detection" >&2 |
There was a problem hiding this comment.
Why have you added output to >&2 here? All debug messages are going to STDOUT. (Same comment for everything below as well).
| notice "Using user defined ${emphasis}AUTOSET_HOSTNAME_SERVICES${reset}=${emphasis}${AUTOSET_HOSTNAME_SERVICES}${reset} for IP detection" >&2 | ||
| else | ||
| debug "Public IP detection will use ${emphasis}${services}${reset} to detect the IP." | ||
| debug "Public IP detection will use ${emphasis}${services[0]}${reset} ... to detect the IP." >&2 |
There was a problem hiding this comment.
This most likely needs to be ${services[*]} to display all elements in an array
There was a problem hiding this comment.
changed this to ${services[*]}
| info "Detected public IP address as ${emphasis}${services}${ip}${reset}." | ||
| break | ||
| info "Detected public IP address as ${emphasis}${ip}${reset} from ${emphasis}${service}${reset}." >&2 | ||
| echo "$ip" |
There was a problem hiding this comment.
Please remove this echo here; it does not align with how the image output logs and it will only confuse users.
| POSTFIX_myhostname="${hostname}" | ||
| else | ||
| POSTFIX_myhostname="${HOSTNAME}" | ||
| fi |
There was a problem hiding this comment.
Thank you for this fix. As you see, we did not have a testcase for this, hence it failed sometime in the past.
Could I kindly bother you to create a unit test as well, so we ensure the code works in the future?
There was a problem hiding this comment.
added/updated unit tests, these mock dig & curl to reduce complexity in the test
…le for IP return, add comprehensive tests
- Remove >&2 redirects from logging functions to follow image
conventions
- Change to use global variable DETECTED_PUBLIC_IP instead of echo
- Fix array expansion: use ${services[*]} instead of ${services[0]}
- Add safety check before using IP in postfix_set_hostname
- Add unit tests
|
Merged! |
postfix_set_hostname:
get_public_ip:
These fixes enable AUTOSET_HOSTNAME=1 to work correctly, auto-detecting the public IP
and setting the Postfix hostname via reverse DNS lookup.