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

Enhance dbinfo sub-command with column names and global variables #70

Merged
merged 12 commits into from
Nov 29, 2024

Conversation

rohit-nayak-ps
Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps commented Nov 24, 2024

  • Rename schema to dbinfo
  • Add column names to returned info
  • Add global variables (small subset for now)

todos

  • Add foreign key info (will be done in future PR)
  • Add test for getting data from a test cluster: currently testing against local mysql
  • Local linter is still failing, find fix and validate code style

@rohit-nayak-ps rohit-nayak-ps force-pushed the rohit/dbinfo branch 2 times, most recently from 297a90c to f427fd6 Compare November 28, 2024 13:17
@rohit-nayak-ps rohit-nayak-ps changed the title [WIP] Enhance dbinfo sub-command Enhance dbinfo sub-command with column names and global variables Nov 28, 2024
@rohit-nayak-ps rohit-nayak-ps marked this pull request as ready for review November 28, 2024 13:33
go/cmd/dbinfo.go Outdated
},
}

cmd.Flags().StringVarP(&vtParams.Host, "host", "", "127.0.0.1", "Database host")
cmd.Flags().IntVarP(&vtParams.Port, "port", "", 3306, "Database port")
cmd.Flags().StringVarP(&vtParams.Uname, "user", "", "root", "Database user")
cmd.Flags().StringVarP(&vtParams.Pass, "password", "", "", "Database password")
cmd.Flags().StringVarP(&vtParams.DbName, "database", "", "", "Database name")
cmd.Flags().StringVarP(&vtParams.DbName, "database", "", "sakila", "Database name")
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably shouldn't use sakila as the default value, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Name string `json:"name"`
Type string `json:"type"`
KeyType string `json:"keyType,omitempty"`
IsNullable bool `json:"isNullable"`
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: could we slim up the json output? like removing isNullable for false values?

Tables []TableInfo `json:"tables"`
FileType string `json:"fileType"`
Tables []*TableInfo `json:"tables"`
GlobalVariables *map[string]string `json:"globalVariables"`
Copy link
Contributor

Choose a reason for hiding this comment

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

why a reference to a map? maps are already sent around by reference, so I'm confused to why

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had started with a different data structure, should have removed the reference once I used maps ... Did.


func (dbh *DBHelper) getGlobalVariables() (*map[string]string, error) {
// Currently only use simple regex to match the variable names
// If the variable name contains ".*" then it is treated as a regex, else exact match
Copy link
Contributor

Choose a reason for hiding this comment

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

seems premature to add the regex functionality, since we are not using it, or have any plans on using it at the moment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@systay
Copy link
Contributor

systay commented Nov 29, 2024

we should also add something to the main README about this functionality

Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
@rohit-nayak-ps
Copy link
Contributor Author

we should also add something to the main README about this functionality

Added some information there. PTAL and modify if required. Also I have auto-wrap setup for md files, if that is not desired I can revert and only update with my changes ...

@systay systay merged commit a2d740d into vitessio:main Nov 29, 2024
1 check 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