feat: add support for domain-scoped projects #185
Conversation
RahulDubey391
requested review from
cthumuluru,
nancynh and
a team
as code owners
RahulDubey391
changed the title
There was a problem hiding this comment.
@RahulDubey391 I took a first pass on the PR, overall its like 80% of the way there 😄
Just need to update based on my suggestions and add tests similar to Python PR 👍
Removed parse_instance_uri() as a class method and instead made it a standalone function. Added test case for the same function to check validity of URI.
There was a problem hiding this comment.
@RahulDubey391 Awesome job on the updates! Couple small tweaks but mostly just adding that missing test for the bad instance URI 😄
Added more tests for _parse_instance_uri(). Made the function private.
| ( | ||
| self._project, | ||
| self._region, | ||
| self._cluster, | ||
| self._name, | ||
| ) = _parse_instance_uri(instance_uri) | ||
|
|
There was a problem hiding this comment.
is the linter adding the () around this tuple? I don't think we need them as the function can just unpack the tuple into the appropriate variables.
| ( | |
| self._project, | |
| self._region, | |
| self._cluster, | |
| self._name, | |
| ) = _parse_instance_uri(instance_uri) | |
| self._project, self._region, self._cluster, self._name = _parse_instance_uri(instance_uri) |
There was a problem hiding this comment.
yes @jackwotherspoon , the tuple was causing the formatting. I have removed it in the latest push.
tests/unit/test_instance.py
Outdated
| ), | ||
| ], | ||
| ) | ||
| def test_parse_instance_uri(instance_uri: str, expected: Tuple[str, str, str]) -> None: |
There was a problem hiding this comment.
I think we need the 4th str for the tuple...
| def test_parse_instance_uri(instance_uri: str, expected: Tuple[str, str, str]) -> None: | |
| def test_parse_instance_uri(instance_uri: str, expected: Tuple[str, str, str, str]) -> None: |
There was a problem hiding this comment.
my bad, I have added and pushed again with latest changes.
tests/unit/test_instance.py
Outdated
| ) | ||
| def test_parse_instance_uri(instance_uri: str, expected: Tuple[str, str, str]) -> None: | ||
| """ | ||
| Test that _parse_instance_uri correctly on |
There was a problem hiding this comment.
| Test that _parse_instance_uri correctly on | |
| Test that _parse_instance_uri works correctly on |
There was a problem hiding this comment.
again, my bad! I have added it.
jackwotherspoon
changed the title
Added missing str in Tuple. Also made changes for unpacking values for parser.
There was a problem hiding this comment.
This is an amazing contribution. Thanks so much @RahulDubey391 👏 🎉
Hi @jackwotherspoon , this is just an initial PR push, I'll be pushing more changes after testing them out. Meanwhile I have added a method for parsing the Instance URI. I got the opportunity to look into the Go Implementation for same issue. I have added the REGEX for the same.
Fixes #184