rfancn / aliyun-ddns-client

Python DDNS client for Aliyun
404 stars 137 forks source link

get IP address from interface. #21

Closed yuguorui closed 6 years ago

yuguorui commented 6 years ago

Added a new filed in the config file in order to get the IP address from the interface setting.

rfancn commented 6 years ago

@yuguorui thanks for your commits!

From my understanding, if you specify the interface name, like "eth0" in config file, it will try to get ip address from eth0:

  1. if this is a intranet IP, there is no need to sync with Aliyun server, so no need of this tool either
  2. If this is a public IP, then your commit can bypass the step to get outside IP from 3322.org, it eliminate one http request operation, but I don't think this operation will take much cost when compared with the codes need added. For next IP detecting are all done by DNS lookup. Of course, if your firewall forbid the access to 3322.org, then this is necessary, but I think this is a rare case, right?

Anyway, if you think this is really useful, please help add more explanation in README, and examples how to set it.

yuguorui commented 6 years ago

Sorry for the indistinction. I have added more explanation and examples in the README and the example config file.

yuguorui commented 6 years ago

BTW, I also adjust the position of import the request package, so the user can get the ip address without having the request package.

rfancn commented 6 years ago

@yuguorui Thanks for your commits.

I assume this is a new feature for multiple NICs with multiple public ip. Then you can specify one of them as the one which will be synced with Aliyun server.

If this is true, please check what I suggested in: https://github.com/rfancn/aliyun-ddns-client/pull/20

Please try to implement as a new feature(e,g: public_ip_from_nic, section name could be: "feature_public_ip_from_nic") and not change get_current_public_ip() , e,g:

if config.feature_pifn_enabled:
  current_public_ip = DDNSUtils.get_interface_address(config.pifn_interface)
else:
  current_public_ip = DDNSUtils.get_current_public_ip()

Also just a correction in README, The requests package still needed as this tool will use it to communicate with Aliyun server.

yuguorui commented 6 years ago

OK, wait a moment.

yuguorui commented 6 years ago

Please review the code ;)

rfancn commented 6 years ago

@yuguorui It's great, thanks for your contribution!

Some suggests list below:

  1. Please change config variable name to follow the rule as below to avoid variable name conflict in the future:
    <feature_name>_<parameter name>
    or
    <feature_abbreivation>_<parameter name>

    e,g: pifn represents as: Public Ip From Nic

    
    FROM:
    self.interface = self.parser.get(section_name, "interface")

TO: self.pifn_interface = self.parser.get(section_name, "interface")


2. Some minor typos in func name

FROM: get_feature_public_ip_from_nic_option

TO: get_feature_public_ip_from_nic_options

yuguorui commented 6 years ago

OK, I have changed the variable names.

rfancn commented 6 years ago

@yuguorui Just small error here :)

As variable name changed, reference here need change too:


FROM:
current_public_ip = DDNSUtils.get_interface_address(config.interface)

TO:
current_public_ip = DDNSUtils.get_interface_address(config.pifn_interface)
yuguorui commented 6 years ago

Sorry for wasting you time, I should use a linter. 😢 I have corrected my foolish mistake by a force commit.