Snow 1181759 git setup #880
Conversation
sfc-gh-pczajka
requested review from
sfc-gh-turbaszek,
sfc-gh-pjob,
sfc-gh-jsikorski,
sfc-gh-astus and
sfc-gh-mraba
as code owners
|
|
||
| manager = GitManager() | ||
|
|
||
| def _assure_repository_does_not_exist() -> None: |
There was a problem hiding this comment.
I would suggest to move those functions outside the command, it will increase readability
| secret_needed = typer.confirm("Use secret for authentication?") | ||
| if not secret_needed: | ||
| return None | ||
|
|
||
| use_existing_secret = typer.confirm("Use existing secret?") | ||
| if use_existing_secret: | ||
| existing_secret = typer.prompt("Secret identifier") | ||
| return existing_secret | ||
|
|
||
| cli_console.step("Creating new secret") | ||
| secret_name = f"{repository_name}_secret" | ||
| username = typer.prompt("username") | ||
| password = typer.prompt("password/token", hide_input=True) | ||
| manager.create_secret(username=username, password=password, name=secret_name) | ||
| cli_console.step(f"Secret '{secret_name}' successfully created") | ||
| return secret_name |
There was a problem hiding this comment.
Inline the prompts and confirm into command. It will be easier to follow the command flow. Currently I find it pretty hard to grasp
| def create_secret(self, name: str, username: str, password: str) -> SnowflakeCursor: | ||
| query = ( | ||
| f"create secret {name}" | ||
| f" type = password" | ||
| f" username = '{username}'" | ||
| f" password = '{password}'" | ||
| ) | ||
| return self._execute_query(query) | ||
|
|
||
| def create_api_integration( |
There was a problem hiding this comment.
Move to SQLMixin as those are not specific to git repo
There was a problem hiding this comment.
Done. I renamed create_secret -> create_password_secret as different secret types have different arguments
sfc-gh-pczajka
force-pushed
the
SNOW-1181759-git-setup
branch
from
55cfc60
to
161800a
Compare
| _assure_repository_does_not_exist(repository_name) | ||
| manager = GitManager() | ||
|
|
||
| url = typer.prompt("Origin url") |
There was a problem hiding this comment.
Should we validate if not empty?
There was a problem hiding this comment.
In that case typer asks again:
▶ snow git setup repo
Origin url:
Origin url:
Origin url:
Origin url:
I was considering validating whether it starts with https://, wdyt?
There was a problem hiding this comment.
+1 for at least https validation
Co-authored-by: Tomasz Urbaszek <[email protected]>
| secret = f"{repository_name}_secret" | ||
| username = typer.prompt("username") | ||
| password = typer.prompt("password/token", hide_input=True) | ||
| manager.create_password_secret( |
There was a problem hiding this comment.
I would move object creation to the end, in this way there's a clear separation of "prompts" and "actions" WDYT?
| query = ( | ||
| f"create git repository {repo_name}" | ||
| f" api_integration = {api_integration}" | ||
| f" origin = '{url}'" | ||
| ) |
There was a problem hiding this comment.
Let's use multiline string, it makes things a bit more readable
…lake-cli into SNOW-1181759-git-setup
| cli_console.step(f"Using existing secret '{secret_name}'") | ||
| else: | ||
| cli_console.step(f"Secret '{secret_name}' will be created") |
There was a problem hiding this comment.
Personally I would hide this information. If secrets exists - just continue. If it does not exists - ask for required value. WDYT?
Pre-review checklist
Changes description
Add
snow git setupcommand, which creates git repository and creates all necessary objects.