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

Added ability to include account value in profiles.yml to specify whi… #4

Merged
merged 3 commits into from Oct 29, 2021

Conversation

ima-hima
Copy link
Contributor

…ch account to use.

Copy link
Contributor

@dataders dataders left a comment

Choose a reason for hiding this comment

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

thanks the wicked fast turn around on this? my suggestions:

  • 2nd pass as query param building logic
  • update CHANGELOG.md
  • accept/ignore my docs suggestions

README.md Show resolved Hide resolved
dbt/adapters/firebolt/connections.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
… profiles.yml. Edited readme to reflect this change. Updated CHANGELOG.md.
Comment on lines 109 to 113
lambda kv: "&"
+ urllib.parse.quote(kv[0])
+ quote(kv[0])
+ "="
+ urllib.parse.quote(kv[1]),
+ quote(kv[1]),
credentials.params.items(),
Copy link
Contributor

Choose a reason for hiding this comment

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

this thing looks like it also adds the query string, starting with &... maybe it could be combined with the url_vars section? maybe i'm being too picky? @miguel-firebolt, what's the use case for params anyway??

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 code is kind of messy. I'll fool around with it for a few minutes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, it's done. Testing needs to be done using parameters, though, which I don't know how to do. @miguel-firebolt can you?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't say I know what the use case for params is...

Copy link
Contributor

@dataders dataders left a comment

Choose a reason for hiding this comment

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

looks good! feel free to ignore/table my suggestion about credentials.params.

in lieu of integration tests, can you confirm that this works?

@ima-hima
Copy link
Contributor Author

looks good! feel free to ignore/table my suggestion about credentials.params.

in lieu of integration tests, can you confirm that this works?

I can confirm this works. I tested using two accounts, using both engines on and one engine off at a time, in order to make sure it was always choosing the correct account.

@ima-hima
Copy link
Contributor Author

Took @swanderz suggestion on adding parameters to jdbc url by using dictionary comprehension.

@ima-hima ima-hima closed this Oct 29, 2021
@ima-hima ima-hima reopened this Oct 29, 2021
@miguel-firebolt
Copy link
Contributor

I also tested and confirmed it works for me
Screen Shot 2021-10-29 at 1 42 10 PM

@dataders dataders merged commit d96b0a6 into main Oct 29, 2021
@ima-hima ima-hima deleted the add_account_name branch November 1, 2021 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants