Skip to content
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

feat(source create snowflake): Support format --account {org}.{account} #206

Merged
merged 3 commits into from
Nov 27, 2023

Conversation

spencerwilson
Copy link
Contributor

@spencerwilson spencerwilson commented Nov 27, 2023

  • Upgrade regress dep
  • Support more user-friendly --account {org}.{account} format when creating Snowflake sources

Test plan

Failure:

% cargo run -- source create snowflake -n testing-easier-identifier --account ASDF --user redacted --password redacted --database redacted
error creating source: Invalid account identifers given. Provide account identifiers either via `--account ${ORG_NAME}.${ACCOUNT_NAME}, or via `--organization ${ORG_NAME} --account ${ACCOUNT_NAME}`.

Success:

% cargo run -- source create snowflake -n testing-easier-identifier --account ABC123.DEF456 --user redacted --password redacted --database redacted
Source created

&& verifying the expected values were persisted:

dpm=> select source_parameters->'organization', source_parameters->'account' from source where name = 'testing-easier-identifier';
 ?column?  | ?column?
-----------+-----------
 "ABC123" | "DEF456"
(1 row)

Copy link
Contributor Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@@ -26,7 +26,7 @@ dialoguer = "0.10.4"
directories = "5.0.1"
inquire = "0.6.2"
prost = "0.11.9"
regress = "0.6.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was motivated by 0.6.0 wrongly rejecting a valid pattern:

^(?<org_name>[a-zA-Z][a-zA-Z0-9]*)[.-](?<account_name>[a-zA-Z_]+)$
thread 'main' panicked at src/command/snowflake.rs:34:6:
called `Result::unwrap()` on an `Err` value: Error { text: "Invalid token at named capture group identifier" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Comment on lines +36 to +43
// SAFETY: Regex match implies that both groups matched.
let org_name = &account_name[result.named_group("org_name").unwrap()];
let account_name = &account_name[result.named_group("account_name").unwrap()];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going forward, I'd like to accompany any non-trivial .unwrap() calls with // SAFETY comments, explaining why they won't panic. The hope is that this practice will promote quality and make it easier to audit the codebase for any preexisting, unjustified unwrapping.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL: Safety comments.

@spencerwilson spencerwilson force-pushed the pat-4859-support-easier-account-ids branch from 908beac to 373912c Compare November 27, 2023 22:47
@spencerwilson spencerwilson merged commit c7175e8 into main Nov 27, 2023
10 checks passed
@spencerwilson spencerwilson deleted the pat-4859-support-easier-account-ids branch November 27, 2023 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants