mysql: pass TLS config directly to MySQL's config #3348
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3348 +/- ##
==========================================
+ Coverage 77.45% 77.53% +0.08%
==========================================
Files 104 104
Lines 13935 13916 -19
==========================================
- Hits 10793 10790 -3
+ Misses 2381 2365 -16
Partials 761 761 ☔ View full report in Codecov by Sentry. |
| dialerCounter.mu.Lock() | ||
| dialerNum := dialerCounter.n | ||
| dialerCounter.mu.Unlock() |
There was a problem hiding this comment.
No increment, it always zero
the `dialerNum` variable doesn't increment in the last implement, it always is zero.
|
Hello @vangent, can you please review? thanks! PS: Oh, sorry. I just remember you're in Thanksgiving holiday |
| @@ -97,12 +98,8 @@ func (uo *URLOpener) OpenMySQLURL(ctx context.Context, u *url.URL) (*sql.DB, err | |||
| if uo.CertSource == nil { | |||
| return nil, fmt.Errorf("gcpmysql: URLOpener CertSource is nil") | |||
| } | |||
| // TODO(light): Avoid global registry once https://github.com/go-sql-driver/mysql/issues/771 is fixed. | |||
There was a problem hiding this comment.
Can you explain why this TODO can be addressed while the issue referenced is still open?
There was a problem hiding this comment.
For this part, it really doesnt have another solution, and the package itself will not support to change this to a custom dialer function. So, I think the current implement is correct and no need to change in future. The package sql-cloud-proxy also used this way without comment
There was a problem hiding this comment.
It would help if you would explain what problem you are trying to solve. Does the current code not work? Or are you trying to simplify it?
Our testing for this part of the codebase is not good, and I am hesitant to make changes that I don't fully understand without any rationale, especially when there's a TODO in the code that hasn't been addressed.
There was a problem hiding this comment.
Hey, hello again.
I was checked the source code of awsmysql and azuremysql and notice about TLSConfig's todo. I checked mysql driver on the upstream repository and it was supported to replace with TLS field
https://github.com/go-sql-driver/mysql/blob/master/dsn.go#L125-L142
for the gcpmysql, the todo comment wasn't correct, because this package use Dial function to make the connection, which no replacement in the feature.
This is another example that use the same approach for Dial:
https://github.com/GoogleCloudPlatform/cloud-sql-go-connector/blob/main/mysql/mysql/mysql.go#L40-L49
And the package gcpmysql also have a bug for generate driver name, it used same name for all connection.
There was a problem hiding this comment.
It would help if you would explain what problem you are trying to solve. Does the current code not work? Or are you trying to simplify it?
There was a problem hiding this comment.
Simplify, and fix one bug.
|
Hello, please let me know if you need anything to review/merge this PR. |
No description provided.