NASA-AMMOS / AIT-DSN

MIT License
19 stars 10 forks source link

Issue #84 - Add SLE parameters to config file #89

Closed Futabay closed 4 years ago

Futabay commented 4 years ago

SLE parameters were added to config.yaml and rcf.py, raf.py and cltu.py were updated accordingly. FYI, I set all protocols version to 4 as a default. Please let me know if I should set rcf and cltu version to 5.

aywaldron commented 4 years ago

Hi @Futabay , thanks for the PR! This looks good. I just noticed in reviewing the code, however, that auth_level is being picked up in common.py before the cltu/rcf/raf classes. Also, the required default should be "none" rather than None: https://github.com/NASA-AMMOS/AIT-DSN/blob/9a266b023368695fb540a598d8d44f88de222a8e/ait/dsn/sle/common.py#L129

Can we change setting the auth_levels within raf/rcf/cltu to default to the already set self._auth_level from the common SLE class? Then that will already have the appropriate "none" default if not specified under dsn.sle.auth_level, basically providing the ability for the individual protocols to override the SLE configuration.

Thanks!

MJJoyce commented 4 years ago

@aywaldron @Futabay,

Sorry to jump on this ticket late and (potentially) ask for a change.

self._auth_level = ait.config.get('dsn.sle.fcltu.auth_level',
                                          kwargs.get('auth_level', self._auth_level))

I think the current config grabs are prioritizing settings differently than we'd like.

I think we'd like the priority to be:

Constructor kwargs > SLE specific auth level > global auth level

It looks like with the current code we're getting

SLE specific auth level > Constructor kwargs > global auth level

Thoughts on changing that priority around?

aywaldron commented 4 years ago

@MJJoyce I agree that switching the priority may be a good idea, yes. The current priority implemented is based on what was already there before working on this ticket, e.g. here, so would require changes beyond this ticket for consistency.

MJJoyce commented 4 years ago

+1, I'm good with that then @aywaldron. Let's just make sure we open up a follow up when we close this out and get the ordering changed.

aywaldron commented 4 years ago

Opened ticket https://github.com/NASA-AMMOS/AIT-DSN/issues/95 for this @MJJoyce and approving & merging this PR.