Skip to content

Add azuresql scheme - #21

Merged
kenshaw merged 2 commits into
xo:masterfrom
burningalchemist:patch-1
Sep 7, 2022
Merged

Add azuresql scheme#21
kenshaw merged 2 commits into
xo:masterfrom
burningalchemist:patch-1

Conversation

@burningalchemist

@burningalchemist burningalchemist commented Aug 31, 2022

Copy link
Copy Markdown
Contributor

Description

The whole point for adding azuresql alias is that recently Microsoft took the ownership for go-mssqldb driver, and by chance they also implemented Azure AD authentication, which implies that on the application side we:

  • imported a subpackage azuread;
  • use azuresql as a scheme.

The rest stays the same, so it seems to be a low hanging fruit.

Reference link: https://github.com/microsoft/go-mssqldb#azure-active-directory-authentication

@kenshaw

kenshaw commented Aug 31, 2022

Copy link
Copy Markdown
Member

I'm OK with adding additional aliases (when they make sense), and this is probably acceptable. Can you please rebase these commits into a single commit? This is a trivial change, and the repo doesn't need 3 separate commits for this.

@burningalchemist

burningalchemist commented Sep 2, 2022

Copy link
Copy Markdown
Contributor Author

Hi @kenshaw, it wasn't ready for review at that time.
Apologies if it created excessive noise for you. It's rebased now. 👍

What do you think?

@burningalchemist
burningalchemist marked this pull request as ready for review September 2, 2022 14:51
@burningalchemist burningalchemist changed the title Add azuresql schema Add azuresql scheme Sep 2, 2022
@kenshaw

kenshaw commented Sep 2, 2022

Copy link
Copy Markdown
Member

@burningalchemist so my concern here is that after reading the documentation, this appears to also require changing to Microsoft's version of the package. I'm OK with that, in principle (assuming Microsoft actually plans to maintain the package), but the drivers need to be switched out in xo and in usql. I'd appreciate PRs to both of those projects changing the Go package/module used, as well as an update to the PR here that changes the preferred driver URL in the documentation (also any documentation references in xo and usql).

@kenshaw

kenshaw commented Sep 2, 2022

Copy link
Copy Markdown
Member

Quick note: for usql, you would only need to change one location in the actual driver (the import line that is tagged with // DRIVER) and then run go generate in the repository root.

@burningalchemist

burningalchemist commented Sep 3, 2022

Copy link
Copy Markdown
Contributor Author

The new package is maintained by the same people, it's also back compatible, so there should be no issues.

I use xo/dburl in an external project, where users are responsible for managing drivers they need. Since the driver is compatible, the only issue I had is recognition of the scheme. Hence the scope of this PR.

I'm happy to update xo and usql accordingly and will provide more PRs soon. Thanks! 👍

UPD: I checked the original repo, and the Azure AD authentication exists there as well. I think it's still a good idea to update to a maintained driver.

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.

2 participants