Drop support of *-sk keys in cc_ssh.#1451
Conversation
Delete *-sk keys from cloud-init-schema.json under
cc_ssh.{ssh_keys,ssh_genkeytypes}
Log a warning if some given key is unsupported or unknown.
Port tests to Pytests, add some types and increase unittest
coverage.
TheRealFalcon
left a comment
TheRealFalcon
left a comment
There was a problem hiding this comment.
Overall looks good. Really nice test changes, though I have some comments inline.
| [mock.call(set(keys), "root", options=options)], | ||
| m_setup_keys.call_args_list, | ||
| cc_ssh.apply_credentials( | ||
| keys, user, *args, disable_root_opts, options, **kwargs |
There was a problem hiding this comment.
I'm a little confused about this call. apply_credentials takes 4 arguments: keys, user, disable_root, and disable_root_opts. Why are you passing an *args and **kwargs too?
There was a problem hiding this comment.
I started the parametrization without knowing if the number of positional args for cc_ssh.apply_credential was variable along the set of tests. After finishing, I should have checked it. Simplified.
| args, | ||
| kwargs, | ||
| ): | ||
| keys, user, *args, disable_root_opts = args |
There was a problem hiding this comment.
Why the *args here? I think this call should just be keys, user, disable_root = args
| keys = ["key1"] | ||
| user = None | ||
| @pytest.mark.parametrize( | ||
| "args,kwargs", |
There was a problem hiding this comment.
Why kwargs? It's an empty dict in all of these definitions, so we shouldn't need it at all.
There was a problem hiding this comment.
I started parametrizing the tests with args and kwargs to be flexible because I did not know how they would be in subsequent tests, but I forgot to remove the redundant things. Simplified!
| @mock.patch(MODPATH + "os.path.exists") | ||
| def test_handle_cfg_without_disable_root( | ||
| self, m_path_exists, m_nug, m_glob, m_setup_keys | ||
| def test_handle_default_root( |
There was a problem hiding this comment.
Nit: I'd rename this to something like test_cfg_disable_root
| class TestHandleSsh(CiTestCase): | ||
| """Test cc_ssh handling of ssh config.""" | ||
| @pytest.fixture(scope="function") | ||
| def _publish_hostkey_test_setup(tmpdir): |
There was a problem hiding this comment.
Nit: I don't think we need to preprend local fixtures with an underscore.
aciba90
left a comment
aciba90
left a comment
There was a problem hiding this comment.
Thank you @TheRealFalcon for pointing out the unneeded flexibility (args, kwargs) and the rest. I have simplified it. Ready to review after the build passes.
| keys = ["key1"] | ||
| user = None | ||
| @pytest.mark.parametrize( | ||
| "args,kwargs", |
There was a problem hiding this comment.
I started parametrizing the tests with args and kwargs to be flexible because I did not know how they would be in subsequent tests, but I forgot to remove the redundant things. Simplified!
| args, | ||
| kwargs, | ||
| ): | ||
| keys, user, *args, disable_root_opts = args |
| [mock.call(set(keys), "root", options=options)], | ||
| m_setup_keys.call_args_list, | ||
| cc_ssh.apply_credentials( | ||
| keys, user, *args, disable_root_opts, options, **kwargs |
There was a problem hiding this comment.
I started the parametrization without knowing if the number of positional args for cc_ssh.apply_credential was variable along the set of tests. After finishing, I should have checked it. Simplified.
2 participants
cc_ssh.{ssh_keys,ssh_genkeytypes}
coverage.
Proposed Commit Message
Additional Context
SC-908
After the discussions in #cloud-init we decided to drop the support
of *-sk key types because:
which does not fit with a cloud-context.
About the integration tests, without the need to support these key types,
I think is sufficient with the existent tests:
Checklist: