Fix sql tool error in windows #7519
Conversation
|
|
| @@ -30,15 +30,13 @@ import ( | |||
| // should not be used for anything more important (password hashes etc.). Marking it as #nosec because of how it's | |||
| // being used. | |||
| "crypto/md5" // #nosec | |||
| "embed" | |||
There was a problem hiding this comment.
i think the idea is to embed the schema into the tool's binary so the schema become optional at runtime, but you can still provide your schema if you want to otherwise it will use the embedded version.
Would this PR break that logic?
There was a problem hiding this comment.
@yiminc
I just gave it a try.
I found that actually embedded schema cannot not be used in windows because the embed filesystem uses "/" while the allowed values of the --schema-name cli option uses "\" on windows (it comes from filepath.Dir(path))
It gives this error on windows
Config Error:(-schema-name) must be one of: [mysql\\v8\\temporal mysql\\v8\\visibility postgresql\\v12\\temporal postgresql\\v12\\visibility]
And using update-schema -s mysql\\v8\\temporal when run into another error.
Do you want to the include a fix for this as well in the PR?
There was a problem hiding this comment.
Yes, we should fix that, I think we want to keep the embed schema working.
There was a problem hiding this comment.
@yiminc
Fixed in embed.go
I updated the readme as well. TBH I didn't know the schema is embedded until you told me, and it looks isn't documented.
san127127
force-pushed
the
fix-sql-tool-error-on-windows
branch
2 times, most recently
from
ff9f308 to
75bee09
Compare
san127127
force-pushed
the
fix-sql-tool-error-on-windows
branch
from
75bee09 to
7c7f69f
Compare
san127127
changed the title
What changed?
Use
path.Join()instead offilepath.Join()Why?
#7454
How did you test it?
Tested locally
Potential risks
None
Documentation
None
Is hotfix candidate?
No