strict checking for context-name in Node Pools to start with "ssh-" #7666
Conversation
There was a problem hiding this comment.
Thanks @pavan-gokarla for the contribution! Left some comments
| elif to_provision.region is not None and to_provision.cloud is not None: | ||
| # For public clouds, provision.region is always set. | ||
| if clouds.SSH().is_same_cloud(to_provision.cloud): | ||
| assert to_provision.region.startswith("ssh-"), "SSH context must start with 'ssh-'" |
There was a problem hiding this comment.
hmm, i thought a user does not need to specify ssh-. Is it true that the region has to be start with ssh-?
There was a problem hiding this comment.
Can you clarify these things, does region need to start with ssh, if yes do we need to strip away the ssh, if it does not start with ssh- we are not required to check right?
| def expand_infras(cls) -> List[str]: | ||
| return [ | ||
| f'{cls.canonical_name()}/{c.lstrip("ssh-")}' | ||
| f'{cls.canonical_name()}/{c.lstrip("ssh-")}' if c.startswith("ssh-") else "." |
There was a problem hiding this comment.
why not just not use lstrip? also, why are we using . when it is not starting with ssh-?
| msg = f'Task{task_name} requires ' | ||
| if region not in existing_contexts: | ||
| if is_ssh: | ||
| assert region.startswith("ssh-"), "SSH context must start with 'ssh-'" |
There was a problem hiding this comment.
let's get rid of using lstrip. Otherwise, ssh-ssabc will become abc
There was a problem hiding this comment.
instead of lstrip will it be ok if i use string slicing?
2 participants
…th "ssh-"
Assertions added for context-name to start with "ssh-"
Tested (run the relevant ones):
bash format.sh/smoke-test(CI) orpytest tests/test_smoke.py(local)/smoke-test -k test_name(CI) orpytest tests/test_smoke.py::test_name(local)/quicktest-core(CI) orpytest tests/smoke_tests/test_backward_compat.py(local)