-
Notifications
You must be signed in to change notification settings - Fork 826
strict checking for context-name in Node Pools to start with "ssh-" #7666
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -252,7 +252,7 @@ def check_single_context(cls, context: str) -> Tuple[bool, str]: | |
| @classmethod | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not just not use lstrip? also, why are we using |
||
| for c in cls.existing_allowed_contexts(silent=True) | ||
| ] | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1289,6 +1289,7 @@ def _check_specified_regions(task: task_lib.Task) -> None: | |
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's get rid of using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of lstrip will it be ok if i use string slicing? |
||
| infra_str = f'SSH/{region.lstrip("ssh-")}' | ||
| else: | ||
| infra_str = f'Kubernetes/{region}' | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, i thought a user does not need to specify
ssh-. Is it true that the region has to be start withssh-?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?