search

NASA-AMMOS / AIT-DSN

MIT License
19 stars 10 forks source link

Hostname / Port configuration options #76

Closed MJJoyce closed 4 years ago

MJJoyce commented 4 years ago

Hostname / port configuration options were removed from the config in 28ae827ee05095216c9e1ef8928eed9d72c4da8c and justification / tracking ticket seems lacking. Investigate and document here or revert if necessary.

Per conversation below, let's update SLE.__init__ to pull from the config as it was before. Default config values can be left as whatever placeholder makes sense.

MJJoyce commented 4 years ago

Hey @kmarwah @aywaldron

It looks like we dropped config options for hostname and port from config.yaml in ait/dsn/sle/common.py with 28ae827ee05095216c9e1ef8928eed9d72c4da8c

Any particular motivation for dropping them? I can't track the changes to a ticket but I'm assuming it's in relation to "CMD and TLM use different hostnames/ports"

Thanks!

aywaldron commented 4 years ago

@MJJoyce They were actually added, and then dropped. If you go to release 1.1.0_alpha they weren't in there. Right now we don't check available hostnames for availability, so these parameters are dynamic when you're doing testing and having them in either the config file or the script itself doesn't make too much difference. I think the plan is to allow for a list of hostnames in the config file when we get around to addressing ticket https://github.com/NASA-AMMOS/AIT-DSN/issues/64.

aywaldron commented 4 years ago

https://github.com/NASA-AMMOS/AIT-DSN/pull/69/commits/36f36684c2b13cfa220064ee3f296d9fcef7ec6a is the commit they were added in, which is part of PR https://github.com/NASA-AMMOS/AIT-DSN/pull/69 (the commit you referenced, https://github.com/NASA-AMMOS/AIT-DSN/commit/28ae827ee05095216c9e1ef8928eed9d72c4da8c is later on in PR https://github.com/NASA-AMMOS/AIT-DSN/pull/69 as well).

MJJoyce commented 4 years ago

Thanks @aywaldron.

The config values are used in 1.0.0_alpha when I look https://github.com/NASA-AMMOS/AIT-DSN/blob/1.1.0_alpha/ait/dsn/sle/common.py#L100

But I realize I may have been a bit vague. I was more interested in why we suddenly stopped pulling the config value in SLE.__init__. Namely here: https://github.com/NASA-AMMOS/AIT-DSN/blob/master/ait/dsn/sle/common.py#L102

I'm fine with not pulling that from the config, and like you mentioned, that will change after #64. Was more interested in why it got dropped since I couldn't find a justification / ticket for it. More book keeping than anything =)

aywaldron commented 4 years ago

@MJJoyce They are referenced there but at runtime they would have defaulted to kwargs.get('hostname', None), and the example scripts passed the hostname/port in as kwargs at runtime rather than including them in the config.

I don't think there is a reason that we stopped pulling from the config there, and I agree that we should revert the init back to what it was before, pulling from the config first and defaulting to kwargs.

MJJoyce commented 4 years ago

+1 regarding your second paragraph, I'll update the parent to reflect that.

MJJoyce commented 4 years ago

Resolved in #83