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.

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

Support connecting with a unix domain socket #286

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

Ramblurr
Copy link
Contributor

@Ramblurr Ramblurr commented Mar 27, 2024

As discussed in #285

This changes the semantics of --server (or the env var, etc) to --server <IP/FQDN | /path/to/socket>

Copy link
Collaborator

@deitch deitch left a comment

Choose a reason for hiding this comment

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

Thanks again for putting this in.

A few comments:

  1. The docs should be updated, specifically the main README and docs/configuration.md
  2. We should have some kind of test for this, even just a unit test of Connection.MySQL()
  3. Should --port be deprecated, and everything move into --server? So you can do --server 127.0.01:3306 or --server 127.0.0.1 or --server /path/to/unix, and `--port will be ignored?

@Ramblurr
Copy link
Contributor Author

1. The docs should be updated, specifically the main README and `docs/configuration.md`

Done!

2. We should have some kind of test for this, even just a unit test of `Connection.MySQL()`

I'm afraid I'm not much a go-lang user myself so I don't feel comfortable adding this, would you be able to add the test? (If there was already a similar test I could modify it).

3. Should `--port` be deprecated, and everything move into `--server`? So you can do `--server 127.0.01:3306` or `--server 127.0.0.1` or `--server /path/to/unix`, and `--port will be ignored?

I don't think that's necessary. Port is already optional, and this change preserves that. Since the go-sql-driver does accept Addr as host:port, it could be done.

I leave that decision up to you as the maintainer.

@Ramblurr
Copy link
Contributor Author

I've enabled the "Allow edits by maintainers" feature so feel free to push to my branch if you want.

@deitch
Copy link
Collaborator

deitch commented Mar 28, 2024

This looks good to me. I won't be able to add enough for the testing for the next week at least, and don't want to hold it up. Existing testing should catch any errors for current functionality, and unix-domain socket is new, so I am good with that.

I approved it. Anything else you want before we merge it in?

@Ramblurr
Copy link
Contributor Author

Ramblurr commented Mar 28, 2024

Existing testing should catch any errors for current functionality, and unix-domain socket is new, so I am good with that.

I did run a local manual test to ensure that I didn't break the tcp connect functionality.. both unix and tcp are working as expected.

I approved it. Anything else you want before we merge it in?

Nope, nothing for this commit. Good to merge.

I've noticed a few documentation inconsistencies that I think are a result of the bash -> golang switch, but I'll open another issue for those.

@deitch deitch merged commit 0fab3eb into databacker:master Mar 28, 2024
2 checks passed
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