-
Notifications
You must be signed in to change notification settings - Fork 7
feat: Identity compatibility #85
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
Conversation
Integration tests passing |
Kudos, SonarCloud Quality Gate passed!
|
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.
LGTM, a couple of comments
steps: | ||
- name: Check out code | ||
uses: actions/checkout@v2 | ||
with: |
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.
What will happen if action is triggered automatically from a push to 0.x branch? Will inputs.branch
have a correct value?
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.
When empty it defaults to the "event" that triggered the run. In this case it will checkout 0.x branch.
https://github.com/actions/checkout#usage
connection = await_only(self.dbapi.connect(*arg, **kw)) # type: ignore[attr-defined] # noqa: F821,E501 | ||
# Synchronously establish a connection that can execute | ||
# asynchronous queries later | ||
conn_func = partial(self.dbapi.connect, *arg, **kw) # type: ignore[attr-defined] # noqa: F821,E501 |
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.
nit: I believe we can just do self.dbapi.connect(*arg, **kw)
instead of using partial
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.
From my testing run
accepts only an async function as a first parameter, not a coroutine object that results from self.dbapi.connect(*arg, **kw)
. It does accept args, but kwargs have to be passed as a partial, I'm afraid.
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.
Docs were merged so I'm merging this PR - firebolt-db/firebolt-db.github.io#170 |
Compatibility with Firebolt 2.0 and firebolt-sdk 1.x