Skip to content

mysql: add support for Azure Database for MySQL#1558

Merged
clausti merged 13 commits intogoogle:masterfrom
vsaroopchand:Add-Azure-MySql
Mar 20, 2019
Merged

mysql: add support for Azure Database for MySQL#1558
clausti merged 13 commits intogoogle:masterfrom
vsaroopchand:Add-Azure-MySql

Conversation

@vsaroopchand
Copy link
Copy Markdown
Contributor

@vsaroopchand vsaroopchand commented Mar 14, 2019

resolves #1305 , adds to #76

Here is my proposal for Azure MySQL. As an FYI, I added an AzureCertFetcher helper to fetch (remote endpoint) or load (local FS) certs.

@googlebot googlebot added the cla: yes Google CLA has been signed! label Mar 14, 2019
clausti
clausti suggested changes Mar 15, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@clausti clausti left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request! I left one comment requesting some code comments for clarity, and can you also please run gofmt?

zombiezen
zombiezen reviewed Mar 15, 2019
View reviewed changes
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 20, 2019 •
edited
Loading

Codecov Report

Merging #1558 into master will decrease coverage by 3.91%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1558      +/-   ##
==========================================
- Coverage   76.74%   72.83%   -3.92%     
==========================================
  Files          70       72       +2     
  Lines        6932     7159     +227     
==========================================
- Hits         5320     5214     -106     
- Misses       1233     1585     +352     
+ Partials      379      360      -19
Impacted Files Coverage Δ
mysql/azuremysql/azuremysql.go 0% <0%> (ø)
azure/azuredb/azuredb.go 0% <0%> (ø)
internal/docstore/dynamodocstore/dynamo.go 0% <0%> (-77.05%) ⬇️
internal/docstore/dynamodocstore/codec.go 15.3% <0%> (-45.36%) ⬇️
blob/blob.go 87.95% <0%> (-2.65%) ⬇️
pubsub/natspubsub/nats.go 76.8% <0%> (-1.13%) ⬇️
pubsub/awssnssqs/awssnssqs.go 84.87% <0%> (-0.98%) ⬇️
internal/docstore/firedocstore/fs.go 67.41% <0%> (-0.95%) ⬇️
pubsub/rabbitpubsub/rabbit.go 73.7% <0%> (-0.7%) ⬇️
internal/docstore/driver/codec.go 73.23% <0%> (-0.59%) ⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01b4568...e5492b7. Read the comment docs.

@vsaroopchand
Copy link
Copy Markdown
Contributor Author

vsaroopchand commented Mar 20, 2019

PTAL

clausti
clausti approved these changes Mar 20, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@clausti clausti left a comment

Choose a reason for hiding this comment

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

My comments have been addressed; since @zombiezen also had comments we'll wait for his sign off before merge. Thanks!

zombiezen
zombiezen reviewed Mar 20, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@zombiezen zombiezen left a comment

Choose a reason for hiding this comment

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

This looks much better, thanks Vishal! One last thing and I'm good from my side:

zombiezen
zombiezen approved these changes Mar 20, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@zombiezen zombiezen left a comment

Choose a reason for hiding this comment

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

LGTM. @clausti, feel free to merge (now that I have the right PR).

@clausti clausti merged commit f039f58 into google:master Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@zombiezen zombiezen zombiezen approved these changes

@clausti clausti clausti approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@zombiezen zombiezen

@clausti clausti

Labels

cla: yes Google CLA has been signed!

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

sql: add support for Azure SQL providers

4 participants

@vsaroopchand @zombiezen @clausti @googlebot