-
Notifications
You must be signed in to change notification settings - Fork 294
feat: add MySQL command support #5749
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
Conversation
@bgajjala8 @RyanDerr @johanbrandhorst Hi! I've addressed all the review feedback:
Ready for re-review! Thank you for the guidance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really cool, thank you! There are a few things we'll need to update before we can merge this:
- We will need to add some documentation to the website, so users can find this new command. Please add a new file called
mysql.mdx
to https://github.com/hashicorp/boundary/tree/main/website/content/docs/commands/connect where the other connect helpers are documented. You should be able to base it on the postgres one. Also update theindex.mdx
document with references to the new document. - We will need to add some tests for this functionality. Fortunately, it's pretty easy to run
mysql
for testing purposes, just as we do withpostgresql
. I think we could add aStartMysql
helper to https://github.com/hashicorp/boundary/blob/main/testing/internal/e2e/infra/docker.go, then create a test similar to https://github.com/hashicorp/boundary/blob/main/testing/internal/e2e/tests/base_plus/target_tcp_connect_postgres_test.go that will connect to the MySQL instance and run some commands. - We'll need to update the CHANGELOG.md with this new feature :). Please follow the existing patterns.
Again, thanks for your contribution to Boundary, this is really exciting!
Co-authored-by: Johan Brandhorst-Satzkorn <[email protected]>
Co-authored-by: Johan Brandhorst-Satzkorn <[email protected]>
Co-authored-by: Johan Brandhorst-Satzkorn <[email protected]>
Fixes content check error by adding mysql.mdx to docs navigation. MySQL page now appears in Commands > connect section alongside postgres.
@bgajjala8 @johanbrandhorst - Updated with all requested changes: Security fixes:
Documentation & Testing:
Ready for re-review! |
testing/internal/e2e/tests/base_plus/target_tcp_connect_mysql_test.go
Outdated
Show resolved
Hide resolved
testing/internal/e2e/tests/base_plus/target_tcp_connect_mysql_test.go
Outdated
Show resolved
Hide resolved
…test.go Co-authored-by: Johan Brandhorst-Satzkorn <[email protected]>
Thanks for the feedback! I've updated the test to be completely self-contained - it now starts its own MySQL container using infra.StartMysql(), eliminates any dependency on environment variables, and actually tests the functionality by executing real MySQL commands (SHOW TABLES, SELECT DATABASE()) through the boundary connect mysql command while validating the connection works end-to-end with proper cleanup of all Docker resources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience. I think the test may fail on some machines because the MySQL output is not accurate. Furthermore, the way our CI is set up at the moment makes it impossible for us to run all the tests against this PR, so I was wondering if I could have your consent for us to recreate this contribution in a PR created by a HashiCorp IBM employee so the CI will run? You will still have the commit credit of course.
I’m happy to give my consent. Please feel free to proceed, and I appreciate you keeping the commit credit. Let me know if I can assist further. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me now! As discussed, lets have @bgajjala8 recreate the PR so we can run CI on it. Thanks for your contribution! We can close this PR out once the other one has been merged.
Sounds great — thanks for the review and the support @bgajjala8 @johanbrandhorst! Looking forward to the merged version. |
@enbiyagoral This feature has been merged! 😄 Thanks for your contribution to Boundary, this was really exciting! Closing this PR out now |
Thanks so much everyone for the help and reviews (@johanbrandhorst, @bgajjala8 )— this was my first contribution to Boundary, and it was a great experience! 🙌 Looking forward to future contributions |
This PR adds MySQL connection support to the Boundary CLI. Users can now securely and easily connect to MySQL targets using the
boundary connect mysql
command.What’s Added?
boundary connect mysql
commandRelated Issue
Closes #5740
Testing
Images
Note: This PR follows the same architecture and UX as the existing helper command for PostgreSQL.