[FEATURE] Add support for --from-dir and providing a dedicated temporary database server
#199
Conversation
bplunkett-stripe
force-pushed
the
bplunkett/plan-flexible-source
branch
from
a618915 to
e1c79cc
Compare
| } | ||
|
|
||
| fmt.Println(planFlags.outputFormat.convertToOutputString(plan)) | ||
| cmd.Println(outputFmt.convertToOutputString(plan)) |
There was a problem hiding this comment.
Canonically, all logs should be done through the command. We were just doing this wrong previously. This is how cobra does it's own logging (e.g., printing usage)
| return err | ||
| } else if len(plan.Statements) == 0 { | ||
| fmt.Println("Schema matches expected. No plan generated") | ||
| cmd.Println("Schema matches expected. No plan generated") |
There was a problem hiding this comment.
Canonically, all logs should be done through the command. We were just doing this wrong previously. This is how cobra does it's own logging (e.g., printing usage)
bplunkett-stripe
force-pushed
the
bplunkett/plan-flexible-source
branch
from
7d25b39 to
ac4fc9e
Compare
| result["host"] = e.sockPath | ||
| result["user"] = e.superuser | ||
| result["port"] = strconv.Itoa(defaultPort) | ||
| result[ConnectionOptionHost] = e.sockPath |
There was a problem hiding this comment.
Exposed so tests can access them for the --{from/to}-empty-dsn parameters
| dsnFlagName string | ||
|
|
||
| // isEmptyDsnUsingPq indicates to connect via DSN using the pq environment variables and defaults. | ||
| isEmptyDsnUsingPq bool |
There was a problem hiding this comment.
This was requested per this PR. This maintains support but makes it more explicit and flexible
bplunkett-stripe
force-pushed
the
bplunkett/plan-flexible-source
branch
from
ac4fc9e to
abfe62d
Compare
bplunkett-stripe
changed the title
--from-dir and providing a dedicated temporary database server
alexaub-stripe
left a comment
alexaub-stripe
left a comment
There was a problem hiding this comment.
Just one comment worth looking at, but nothing blocking! Looks great!
| if err != nil { | ||
| return planConfig{}, err | ||
| func parseSchemaSource(p schemaSourceFactoryFlags) (schemaSourceFactory, error) { | ||
| // Store result in a var instead of returning early to ensure only one option is set. |
There was a problem hiding this comment.
Couldn't you just early return if not len(p.schemaDirs) > 0 XOR p.connFlags.IsSet()? Should remove a couple of the ssf nil check error blocks below.
There was a problem hiding this comment.
I prefer this approach because it scales slightly better. If you put the condition at the top like you suggest then we are duplicating the condition, meaning they can become out of alignment.
You can imagine this gets more painful if we scale to 3 separate cases. Imagine a new schema source type that we use if foobarParams != "". We would then need to add it in two separate places with the approach you suggest
There was a problem hiding this comment.
Fine to go with this, though I think you sacrifice some readability in order to avoid that duplication of the checks.
Half joking but I wonder if pushing ssf's onto a result slice and then waiting til the end to check if there isn't exactly one would be the best of both worlds. No need to do ssf != nil checks on each case.
There was a problem hiding this comment.
Fine to go with this, though I think you sacrifice some readability in order to avoid that duplication of the checks.
Too true
Half joking but I wonder if pushing ssf's onto a result slice and then waiting til the end to check if there isn't exactly one would be the best of both worlds. No need to do ssf != nil checks on each case.
I half agree 😎
Description
Changes:
cobra.CommandproxyMotivation
#198
Testing
Added tests for these core commands because it was getting too complicated to run manually. This does not all cover all cases, but it covers core usages.