Skip to content

'or' logic doesn't allow 'None' which evaluates as False.#798

Closed
jjico wants to merge 3 commits into
Juniper:masterfrom
jjico:master
Closed

'or' logic doesn't allow 'None' which evaluates as False.#798
jjico wants to merge 3 commits into
Juniper:masterfrom
jjico:master

Conversation

@jjico

@jjico jjico commented Nov 22, 2017

Copy link
Copy Markdown

This means that "allow_agent" cannot be set to True
based on the logic provided.

Instead: check if the value exists in the dict, then
use the value absolutely if set (even if it's "None").

This means that "allow_agent" cannot be set to True
based on the logic provided.

Instead, check if the value exists in the dict, then
set the value absolutely (even if it's "None") if set.
@jnpr-community-netdev

Copy link
Copy Markdown

Autobot: Would an admin like to run functional tests?

@coveralls

coveralls commented Nov 22, 2017

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.3%) to 97.947% when pulling 226ac24 on jjico:master into 2eaac6c on Juniper:master.

@jnpr-community-netdev

Copy link
Copy Markdown

Can one of the admins verify this patch?

@coveralls

coveralls commented Nov 29, 2017

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.03%) to 98.309% when pulling ff05759 on jjico:master into 2eaac6c on Juniper:master.

@jjico

jjico commented Nov 29, 2017

Copy link
Copy Markdown
Author

This failed because paramiko returned "zero length field name in format", which is an old python2.6-ism with format strings. The failure wasn't with the patch itself. Paramiko updated about 12 days ago and paramiko 2.X no longer supports python 2.6 ( paramiko/paramiko#1070 ). I submitted a patch for this and Travis seems to have accepted it.

Separately I agree with paramiko and think it's reasonable to drop python2.6 support, but that's not mine to decide.

Please re-evaluate.

@vnitinv

vnitinv commented Dec 5, 2017

Copy link
Copy Markdown
Contributor

@jjico Can you please explain how or operator is evaluating to False. None or None will only give None.

@jjico

jjico commented Dec 5, 2017

Copy link
Copy Markdown
Author

If you have a "Host *" entry in your ~/.ssh/config, then "self._conf_ssh_private_key_file" will be set to the sshkey provided there, paramiko correctly finds it for you.

@vnitinv vnitinv requested a review from stacywsmith December 6, 2017 04:58
@jjico

jjico commented Apr 1, 2019

Copy link
Copy Markdown
Author

The issue remains but won't be resolved without a massive rebase. Closing this pull request without merging.

@jjico jjico closed this Apr 1, 2019
@rsmekala

rsmekala commented Apr 2, 2019

Copy link
Copy Markdown
Contributor

@jjico This feature request will be taken in #920. Can you please provide your comments, if you need anything more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants