Changed add_token.py for issue #4526 #4546
Conversation
There was a problem hiding this comment.
Thanks a lot @ShrishtiKarkera! This is going in the right direction, and solves the issue at hand. However, there are things that can be improved, please have a look at the comments.
scripts/wrappers/add_token.py
Outdated
| addr = str(ip) + ":" + str(port) + "/" + str(token) + "/" + str(check) | ||
| if addr not in addr_set: | ||
| print(f"microk8s join {ip}:{port}/{token}/{check}") | ||
| addr_set.add(addr) |
There was a problem hiding this comment.
This is creating a set of IPs we have already "seen". However, the ask here is to simply avoid repeating the default_ip. It should be simpler if you simply check for ip == default_ip in the for loop, right?
There was a problem hiding this comment.
Alright!
I was thinking if the there were duplicate ips in the all_ip list apart from default ip but its much simpler if the default ip is the only problem.
Thanks!
tests/unit/test_addtoken.py
Outdated
| @mock.patch('builtins.print') | ||
| @mock.patch('add_token.get_network_info', return_value=['10.23.53.54', ['10.23.53.54'], str(32)]) | ||
| def test_add_token(self, addtoken, mock_print): | ||
| # self.assertEqual(print_short('t', 'c'), 2) |
There was a problem hiding this comment.
is this leftover from debugging? do we need it?
tests/unit/test_addtoken.py
Outdated
| from unittest import mock, TestCase | ||
| from add_token import print_short | ||
|
|
||
| class test_at(TestCase): |
There was a problem hiding this comment.
test classes are typically named with camel-case: TestSomething. Please use a meaningful name for what we test here.
There was a problem hiding this comment.
My bad, test_at meant test_addtoken but I'll be more clearer.
Thanks
tests/unit/test_addtoken.py
Outdated
|
|
||
| class test_at(TestCase): | ||
| @mock.patch('builtins.print') | ||
| @mock.patch('add_token.get_network_info', return_value=['10.23.53.54', ['10.23.53.54'], str(32)]) |
There was a problem hiding this comment.
let's be consistent and use '32' as well, instead of str(32)
tests/unit/test_addtoken.py
Outdated
| @mock.patch('add_token.get_network_info', return_value=['d_ip', ['ip1', 'ip2', 'd_ip'], '4']) | ||
| def test_multiple(self, multiple, mock_print): | ||
| print_short('t', 'c') | ||
| mock_print.assert_called_with('microk8s join ip2:4/t/c') |
There was a problem hiding this comment.
this works, but does not tell the whole story. please make sure that you test all print invocations are as expected, not only the last one.
tests/unit/test_addtoken.py
Outdated
| all_outputs[0] == f"microk8s join d_ip:4/t/c" | ||
| assert all_outputs[1] == f"microk8s join ip1:4/t/c" | ||
| assert all_outputs[2] == f"microk8s join ip2:4/t/c" | ||
| assert len(all_outputs) == 3 |
There was a problem hiding this comment.
this can be a single assert all_outputs == [cmd1, cmd2, cmd3]
scripts/wrappers/join.py
Outdated
| if "Address" in data: | ||
| port = data["Address"].rsplit(":")[-1] | ||
| except OSError: | ||
| pass | ||
|
|
There was a problem hiding this comment.
this is unrelated to the changes of this PR, please revert
There was a problem hiding this comment.
I'm so sorry, I'll fix this
scripts/wrappers/join.py
Outdated
| if "Address" in data: | ||
| port = data["Address"].rsplit(":")[-1] | ||
| except OSError: | ||
| pass | ||
|
|
There was a problem hiding this comment.
I'm so sorry, I'll fix this
tested with unittest (test_addtoken.py) tested with unittest (test_addtoken.py) changed and tested add_token.py canonical#4526 changed and tested add_token.py checked test_multiple for all outputs changed assertion in test_multiple() checked lint and cla checking for lint and cla checking for cla checking for cla checking for cla checking final checking final final push
Summary
Changes
/microk8s/scripts/wrappers/add_token.py
Testing
The change was tested with a unittest in tests/unit/test_addtoken.py
Tested using mock print assertion with builtins.print
Possible Regressions
Checklist
Notes