Modern POSIX style CLI for SQLCMD #165
Conversation
stuartpa
force-pushed
the
stuartpa/modern-cli
branch
6 times, most recently
from
cb6c769 to
9e5e33f
Compare
stuartpa
force-pushed
the
stuartpa/modern-cli
branch
from
9e5e33f to
af30136
Compare
| // main is the entrypoint function for sqlcmd. | ||
| // | ||
| // TEMPORARY: While we have both the new cobra and old kong CLI | ||
| // implementations, main decides which CLI framework to use |
There was a problem hiding this comment.
i suspect some version of this code is permanent. We aren't likely to reproduce all the legacy switches in the new CLI are we? Perhaps we should use an environment variable to only allow modern mode where it uses query as the default command but only with whatever switches that command supports.
#Resolved
There was a problem hiding this comment.
I think we can solve the problem of the legacy switches by writing a small "canonicalizer" called at the start of main, which scans through the supplied args, and modifies them to be parsable by Cobra, then call cobra.command.SetArgs to override what's passed in from env.
I'll look at this staight after this PR is in. I've entered an issue and assigned it to myself.
| LoggingLevel: 2, | ||
| } | ||
|
|
||
| config.SetFileName(configFilename) |
There was a problem hiding this comment.
It's set the default value for the --sqlconfig flag, so it's ~.sqlcmd\sqlconfig (this func is called by the cobra framework after the flag defaults have been set
| // to make progress. displayHints is injected into dependencies (helpers etc.) | ||
| func displayHints(hints []string) { | ||
| if len(hints) > 0 { | ||
| output.Infof("\nHINT:") |
There was a problem hiding this comment.
Good question. I don't know if all the output renderers on Windows do the right thing. Should i replace this with SqlcmdEol to be safe?
There was a problem hiding this comment.
I've replaced all to sqlcmd.SqlcmdEol for next PR (in sheuybubble/bcp) (which I'll then need to work out how to inject, rather than having everything take an import on sqlcmd package)
| func verifyConfigIsEmpty(t *testing.T) { | ||
| if !config.IsEmpty() { | ||
| bytes := output.Struct(config.GetRedactedConfig(true)) | ||
| t.Errorf("Config is not empty. Content of config file:\n%s\nConfig file used:%s", |
There was a problem hiding this comment.
Added to:
Should tests standarize on a specific assert library e.g. github.com/stretchr/testify/assert
Also in the Go Style Guide, what does "invalid local style" / "Use of assertion-based testing libraries" mean:
https://google.github.io/styleguide/go/guide#local-consistency
"
Examples of invalid local style considerations:
Line length restrictions for code
Use of assertion-based testing libraries
"
The following PR will refactor these tests to what ever we decide
| }() | ||
| cmd.Execute() | ||
| } | ||
| cmd.Execute() |
There was a problem hiding this comment.
is silence considered success? The lack of asserts makes it hard to tell what's being tested. #Resolved
There was a problem hiding this comment.
Agree, I was focusing on getting to 100% code coverage, but now I need to go back and actually validate the test results. To keep the size of the PR managable I'll follow up now with a Test focused PR.
| func (c *Cmd) CheckErr(err error) { | ||
| // If we are in a unit test driver, then panic, otherwise pass down to cobra.CheckErr | ||
| if strings.HasSuffix(os.Args[0], ".test") || // are we in go test? | ||
| (len(os.Args) > 1 && os.Args[1] == "-test.v") { // are we in goland unittest? |
There was a problem hiding this comment.
this seems like a flimsy convention considering someone else could write a new test framework etc. Can these test frameworks not set some global state instead of relying on command line parsing? #Resolved
There was a problem hiding this comment.
Added issue to track
| - container: | ||
| id: 0e698e65e19d9c | ||
| image: mcr.microsoft.com/mssql/server:2022-latest | ||
| endpoint: |
There was a problem hiding this comment.
Let's pretend we will have scenarios involving non-TDS endpoints (eg PowerBI, postgres, SQL AS/RS/IS). Will adding those just be a matter of adding an optional "type" field to "endpoint" to identify it as needing a different driver?
theoretically sqlcmd query "some query" could run any arbitrary query language if the query command were to route it to a package that knows how to connect to the non-TDS endpoint. #Resolved
There was a problem hiding this comment.
I love the growth mind set here! 😄
There was a problem hiding this comment.
Related to this, to be able the best connection strings from:
sqlcmd config connection-strings
Do we need to keep some metadata for the endpoint that tells us it is mssql-server, or mssql-edge, or azsqldb, or postgres! So we know how to generate a good connection string for it
There was a problem hiding this comment.
| Image string `mapstructure:"image"` | ||
| } | ||
|
|
||
| type AssetDetails struct { |
There was a problem hiding this comment.
They're called "struct tags", and you're right to ask, because they are barely documented in the golang spec:
This answers to this stackoverflow question seems to do the best at gathering the links to learn more:
https://stackoverflow.com/questions/10858787/what-are-the-uses-for-struct-tags-in-go
| err, _ := printf("Hope this works) | ||
| checkErr(err) | ||
| - Do not have an internal package take a dependency on another internal package | ||
| unless they are building on each other, instead inject the needed capability in the |
| var loggingLevel verbosity.Enum | ||
| var runningUnitTests bool | ||
|
|
||
| var standardWriteCloser io.WriteCloser |
There was a problem hiding this comment.
don't we need separate error and stdout channels? Most apps don't write everything to stdout, they use stderr too.
Legacy Sqlcmd also lets the user change where stdout and stderr write during app execution. Do you expect we'll keep that functionality with the new API such that this channel needs to support dynamic reassignment? #Resolved
There was a problem hiding this comment.
I started with two channels, but then didn't know what semantics to apply to do the separation, so I removed stderr to keep this PR contained. I've added an Issue, and assigned to myself:
| func (f *Xml) Serialize(in interface{}) (bytes []byte) { | ||
| var err error | ||
|
|
||
| bytes, err = xml.MarshalIndent(in, "", " ") |
There was a problem hiding this comment.
Done for both this one (XML) and Json
|
|
||
| package verbosity | ||
|
|
||
| type Enum int |
There was a problem hiding this comment.
I get what you're saying, but I've done it this way so it looks like this when using it:
output.New(verbosity.Enum(options.LoggingLevel))
golang uses the package name to add the context.
But agree this might be me trying to make it familiar to a C# user.
I'll change it to verbosity.Level (I'll do this in the next PR)
| var sb strings.Builder | ||
| for _, v := range vars { | ||
| sb.WriteString(envVarCommand()) | ||
| sb.WriteString(cliQuoteIdentifier() + v + cliQuoteIdentifier()) |
There was a problem hiding this comment.
is there really value in splitting these up instead of having a single function that takes the variable name?
This function could accept a map instead of an array, and pass the keys as the var names and the values as the var values. The pal function that exports the environment variable would parse the values and do any escaping of special characters.
#Resolved
There was a problem hiding this comment.
The value is code coverage reporting 😄 . I'd like to promote a culture that a lot of the files in this repo are at 100% "go test" code coverage (already "most" files are), (I want to add a "100PercentClub" build pipeline check so a list (or source file metadata tag?) of all files that are at 100% code coverage to never drop below 100%).
There is 2nd reason, the other approach is to use runtime.GOOS == "windows" etc., but this generates a style failure in go vet (and IDEs), because it is flagged as "unreachable" so you never get the little "green tick" for the source file being 100% good.
There was a problem hiding this comment.
I've added issue:
Protect source files already at 100% code coverage - add "100PercentClub" build pipeline check to
There was a problem hiding this comment.
I've added comment to:
"Another topic around coding guidelines around Platform Abstraction, should we use runtime.GOOS == "windows" etc. or the filename_windows, filename_darwin, filename_linux approach:"
There was a problem hiding this comment.
I'll refactor func to take a map in next PR
There was a problem hiding this comment.
I like using the file_linux (or build tags) approach so per-platform code isn't intermixed.
I wasn't asking to merge all the functions into one using the GOOS=="windows" check, I just meant to merge the envVarCommand() and cliQuoteIdentifier() concatenations into a single function. That function would take both the name and the value of the variable so it can properly escape the value for the current platform.
| } | ||
|
|
||
| inRune := []rune(password.String()) | ||
| mathRand.Shuffle(len(inRune), func(i, j int) { |
There was a problem hiding this comment.
interview question - implement this function! #Resolved
| }) | ||
| } | ||
|
|
||
| func Initialize(handler func(err error)) { |
There was a problem hiding this comment.
I've added a more general issue to cover this area of style guidelines. I think I agree with you (this is what I'd do in c#), but would using interface encourage tighter coupling. Is the idomatic golang apporach for the caller to define the interface.
| ) | ||
| conn, err := net.DialTimeout( | ||
| "tcp", | ||
| net.JoinHostPort("localhost", strconv.Itoa(port)), |
There was a problem hiding this comment.
Done (added to the follow on PR in shueybubbles/bcp)
| } | ||
|
|
||
| if c.authType == "basic" { | ||
| if os.Getenv("SQLCMD_PASSWORD") == "" { |
There was a problem hiding this comment.
Added issue and assigned to myself (to break this PR up a bit):
| func (c *ConnectionStrings) run() { | ||
| // connectionStringFormats borrowed from "portal.azure.com" "connection strings" pane | ||
| var connectionStringFormats = map[string]string{ | ||
| "ADO.NET": "Server=tcp:%s,%d;Initial Catalog=%s;Persist Security Options=False;User ID=%s;Password=%s;MultipleActiveResultSets=False;Encode=True;TrustServerCertificate=False;Connection Timeout=30;", |
There was a problem hiding this comment.
Added more general issue to review these connection strings:
"Review sqlcmd config connection-strings"
| func (c *MssqlBase) Run() { | ||
| var imageName string | ||
|
|
||
| if !c.acceptEula && viper.GetString("ACCEPT_EULA") == "" { |
There was a problem hiding this comment.
Is I am using the viper feature to bind to env here ( err = viper.BindEnv("ACCEPT_EULA")), but no binding to the deep sub cmd flag. The reason I didn't do that is the flag is deep down in the hierarchy, e.g. sqlcmd install mssql --flag, which would be a long variable name, someting like SQLCMD_INSTALL_MSSQL_ACCEPT_EULA I think)
|
Thanks for the all the PR feedback. I'll follow up with a quick PR to neaten the unit tests. (I was focused on getting most files to 100% code coverage, need to go back and validate/assert result output). In reply to: 1196841042 |
|
Could SqlcmdEol declaration move to internal?
Sent from Outlook<http://aka.ms/weboutlook>
________________________________
From: Stuart Padley ***@***.***>
Sent: Tuesday, November 29, 2022 4:43 AM
To: microsoft/go-sqlcmd ***@***.***>
Cc: David Shiflet ***@***.***>; Comment ***@***.***>
Subject: Re: [microsoft/go-sqlcmd] Modern POSIX style CLI for SQLCMD (PR #165)
@stuartpa commented on this pull request.
+// To aid debugging issues, if the logging level is > 2 (e.g. -v 3 or -4), we
+// panic which outputs a stacktrace.
+func checkErr(err error) {
+ if loggingLevel > 2 {
+ if err != nil {
+ panic(err)
+ }
+ }
+ rootCmd.CheckErr(err)
+}
+
+// displayHints displays helpful information on what the user should do next
+// to make progress. displayHints is injected into dependencies (helpers etc.)
+func displayHints(hints []string) {
+ if len(hints) > 0 {
+ output.Infof("\nHINT:")
I've replaced all to sqlcmd.SqlcmdEol for next PR (in sheuybubble/bcp) (which I'll then need to work out how to inject, rather than having everything take an import on sqlcmd package)
—
Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2Fgo-sqlcmd%2Fpull%2F165%23discussion_r1034515996&data=05%7C01%7Cdavid.shiflet%40microsoft.com%7Cc1f4d0319d944df3ff7908dad1ee2129%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C638053117931304099%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=s%2Fm%2B9kh5pMp%2BhAMnnXa1YPdBjryIuHMxTNU1lMq%2FhfM%3D&reserved=0>, or unsubscribe<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAAQ7GCVW5NKQMG5YJE5DYVLWKXF27ANCNFSM6AAAAAASH3EPBE&data=05%7C01%7Cdavid.shiflet%40microsoft.com%7Cc1f4d0319d944df3ff7908dad1ee2129%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C638053117931304099%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=kgx9%2Boj0C9eKNnRImJodsrzM0LW7s2KYzkTcc7%2FetOc%3D&reserved=0>.
You are receiving this because you commented.Message ID: ***@***.***>
|
Main package (main.go) is the entry point for the sqlcmd CLI application.
To follow the flow of this code:
cobra CLI
NewtheRootcmd (and all itssubcommands)
the logging level, config file path, error handling and trace support passed
into internal packages)
function for the command (sqlcmd install, sqlcmd query etc.) being run
runmethod that performs the actionhandling and trace (non-localized) logging (as can be seen from the
importfor each command (in /cmd/root/...)).
This code follows the Go Style Guide