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.

Sign up for GitHub

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

Jump to bottom

Expose dbsql.connOption type #202

Merged
merged 1 commit into from
Jul 25, 2024
Merged

Expose dbsql.connOption type #202

merged 1 commit into from
Jul 25, 2024

Conversation

shelldandy
Copy link
Contributor

@shelldandy shelldandy commented Apr 8, 2024 •
edited
Loading

Fixes: #201

That way we can programmatically add arguments to the NewConnector function instead of copy/pasting all of them across conditionals.

@shelldandy
expose connOption type
0151666 
Signed-off-by: Miguel Palau <[email protected]>
@shelldandy
Copy link
Contributor Author

Just did a force push to add the sign-off message

@shelldandy
Copy link
Contributor Author

@kravets-levko could i get a review now? i added the sign off message :)

@kravets-levko
Copy link
Contributor

Hi @shelldandy! The code on its own looks good, but can you please explain the use case for this change? Maybe some examples how it may be useful? Thank you!

@shelldandy
Copy link
Contributor Author

Hi @shelldandy! The code on its own looks good, but can you please explain the use case for this change? Maybe some examples how it may be useful? Thank you!

Of course, yeah this is to help in programmatically creating a connection via a settings page so by having the connOption type exposed that means we can use conditionals to add to the arguments passed to the new connector function instead of having to duplicate code each time we hit a new part of the conditional because the type is not exported and the new connector function only allows for arguments of that type.

@shelldandy
Copy link
Contributor Author

@kravets-levko please review again, so an use case for example is if we're programatically creating a connector so if in this case we sometimes need to have a connector withtransport but because the type is not exposed currently we're unable to just add arguments before doing the NewConnector call:

image

Having this PR merged can help us DRY the code a whole lot.

kravets-levko
kravets-levko approved these changes Jul 25, 2024
View reviewed changes
Copy link
Contributor

@kravets-levko kravets-levko left a comment

Choose a reason for hiding this comment

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

@shelldandy Thank you for the example, now I indeed see the value of exporting this type. Waiting for the tests to run, and if no issues - will merge. Thank you for your effort on this!

@kravets-levko kravets-levko merged commit f0e3a08 into databricks:main Jul 25, 2024
0 of 3 checks passed
@kravets-levko kravets-levko mentioned this pull request Jul 25, 2024
Merged
kravets-levko added a commit that referenced this pull request Jul 30, 2024
@kravets-levko
Fix missed connOption when exporting ConnOption type (#238)
fe1beea 
Follow up on #202

Signed-off-by: Levko Kravets <[email protected]>
atzoum pushed a commit to rudderlabs/sqlconnect-go that referenced this pull request Aug 20, 2024
@dependabot
feat(databricks): bump github.com/databricks/databricks-sql-go from 1…
14852fe 
….5.7 to 1.6.0 (#154)

[//]: # (dependabot-start)
⚠️  **Dependabot is rebasing this PR** ⚠️ 

Rebasing might not happen immediately, so don't worry if this takes some
time.

Note: if you make any changes to this PR yourself, they will take
precedence over the rebase.

---

[//]: # (dependabot-end)

Bumps
[github.com/databricks/databricks-sql-go](https://github.com/databricks/databricks-sql-go)
from 1.5.7 to 1.6.0.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a
href="https://github.com/databricks/databricks-sql-go/releases">github.com/databricks/databricks-sql-go's
releases</a>.</em></p>
<blockquote>
<h2>v1.6.0</h2>
<ul>
<li><code>databricks/databricks-sql-go#233</code><a
href="https://github.com/anthonycrobinson"><code>@​anthonycrobinson</code></a>)</li>
<li>Expose
<code>dbsql.ConnOption</code><code>databricks/databricks-sql-go#202</code><a
href="https://github.com/shelldandy"><code>@​shelldandy</code></a>)</li>
<li><code>databricks/databricks-sql-go#240</code><a
href="https://github.com/jackyhu-db"><code>@​jackyhu-db</code></a>)</li>
</ul>
<p><strong>Full changelog:</strong> <a
href="https://github.com/databricks/databricks-sql-go/compare/v1.5.7...v1.6.0">https://github.com/databricks/databricks-sql-go/compare/v1.5.7...v1.6.0</a></p>
</blockquote>
</details>
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/databricks/databricks-sql-go/blob/main/CHANGELOG.md">github.com/databricks/databricks-sql-go's
changelog</a>.</em></p>
<blockquote>
<h2>v1.6.0 (2024-07-31)</h2>
<ul>
<li><code>databricks/databricks-sql-go#233</code><a
href="https://github.com/anthonycrobinson"><code>@​anthonycrobinson</code></a>)</li>
<li>Expose
<code>dbsql.ConnOption</code><code>databricks/databricks-sql-go#202</code><a
href="https://github.com/shelldandy"><code>@​shelldandy</code></a>)</li>
<li><code>databricks/databricks-sql-go#240</code><a
href="https://github.com/jackyhu-db"><code>@​jackyhu-db</code></a>)</li>
</ul>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/databricks/databricks-sql-go/commit/93222abd7f5be64af98bd90fe8d60972d365eda6"><code>93222ab</code></a>
Prepare release v1.6.0 (<a
href="https://redirect.github.com/databricks/databricks-sql-go/issues/241">#241</a>)</li>
<li><a
href="https://github.com/databricks/databricks-sql-go/commit/12eeae81ea89bc7b75301aa49fbab1ef3ba52e0d"><code>12eeae8</code></a>
[BL-8235] Fix a connection leak in PingContext (<a
href="https://redirect.github.com/databricks/databricks-sql-go/issues/240">#240</a>)</li>
<li><a
href="https://github.com/databricks/databricks-sql-go/commit/fe1beeae91153da14fdeea105a03d0cf169a2ddc"><code>fe1beea</code></a>
Fix missed connOption when exporting ConnOption type (<a
href="https://redirect.github.com/databricks/databricks-sql-go/issues/238">#238</a>)</li>
<li><a
href="https://github.com/databricks/databricks-sql-go/commit/f0e3a08475e58316c2f6141590eba5899a4ef693"><code>f0e3a08</code></a>
Expose dbsql.connOption type (<a
href="https://redirect.github.com/databricks/databricks-sql-go/issues/202">#202</a>)</li>
<li><a
href="https://github.com/databricks/databricks-sql-go/commit/a21b1248382f7132da5aa4cdcba8eedc1a920a67"><code>a21b124</code></a>
Remove secrets (<a
href="https://redirect.github.com/databricks/databricks-sql-go/issues/236">#236</a>)</li>
<li><a
href="https://github.com/databricks/databricks-sql-go/commit/bd10b6262c88da22a7ba09737fe64b1091c2476c"><code>bd10b62</code></a>
fix(security): Bump x/net to v0.17 to resolve vulnerability (<a
href="https://redirect.github.com/databricks/databricks-sql-go/issues/233">#233</a>)</li>
<li>See full diff in <a
href="https://github.com/databricks/databricks-sql-go/compare/v1.5.7...v1.6.0">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=github.com/databricks/databricks-sql-go&package-manager=go_modules&previous-version=1.5.7&new-version=1.6.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kravets-levko kravets-levko kravets-levko approved these changes

@yunbodeng-db yunbodeng-db Awaiting requested review from yunbodeng-db yunbodeng-db is a code owner

@andrefurlan-db andrefurlan-db Awaiting requested review from andrefurlan-db andrefurlan-db is a code owner

@rcypher-databricks rcypher-databricks Awaiting requested review from rcypher-databricks rcypher-databricks is a code owner

@jadewang-db jadewang-db Awaiting requested review from jadewang-db jadewang-db is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expose dbsql.connOption type
2 participants
@shelldandy @kravets-levko