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

A list of paths can be past in for schemas and queries #426

Merged
merged 2 commits into from
Apr 5, 2020

Conversation

jskelcy
Copy link
Contributor

@jskelcy jskelcy commented Apr 1, 2020

This adds support for passing in a list of files or directories for queries and schemas.

This came about because the project currently recommends workarounds for SQL views, where users create a table definition that is the would-be table schema for the view. We did not want this file living alongside our production schema so I created an adjacent director called workarounds, this was going to make scripting fresh DB migrations much simpler as I did not have to regex out workaround files. However, because I could only pass in one path this did not work. This adds that support.

It is kind of massive and breaks config backwards compatibility but I can look into options for fixing that.

@jskelcy
Copy link
Contributor Author

jskelcy commented Apr 1, 2020

So I think I can create a new type with a custom yaml and json umarshaler. I did most of this with find and replace but this is way too massive of a PR.

@kyleconroy
Copy link
Collaborator

@jskelcy thanks for the patch!

Since this is a backwards-incompatible change, we'd need to bump the configuration file version. The current version is '2', though it's not in heavy use. Version '1' is what most people use.

That said, I think we can solve this in a backwards compatible way using glob patterns.

version: "1"
packages:
  - name: "db"
    path: "internal/db"
    schema: "./migrations/*"
    queries: "./sql/query/"

Here the migrations directory would have two directories in it, one with the production schema and one with the extra one for views.

Would that work for you?

Also I'm sorry that I haven't made any progress on #193, view support is still pretty far off

@jskelcy
Copy link
Contributor Author

jskelcy commented Apr 2, 2020

@kyleconroy I made the change backward compatible let me know what you think.

Also I was thinking about the view thing, and yeah that seems super complicated. Views don't have type definitions, so that is kind of tricky.

Finally, I really love this project, thanks so much for all the work you have been doing.

@jskelcy
Copy link
Contributor Author

jskelcy commented Apr 2, 2020

I can also mess around with the glob patterns, it might work out fine for me. But I figured I had already got this thing in some kind of shape, so why I would make it backwards compatible and see how we felt about it.

@jskelcy jskelcy force-pushed the js/sql_file_lists branch 2 times, most recently from aed157b to 48252b6 Compare April 2, 2020 03:09
@@ -35,6 +36,39 @@ type versionSetting struct {

type Engine string

type Paths []string

func (p *Paths) UnmarshalJSON(data []byte) error {
Copy link
Contributor Author

@jskelcy jskelcy Apr 2, 2020

Choose a reason for hiding this comment

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

I am not sure that I need this function. When the tests were failing they contained yaml parsing errors even when the tests appeared to use json. Still trying to figure that out, but until I understand that there is some conversion or something going on then I will leave this here for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We technically don't need this, as we use the yaml package to parse both JSON and YAML. However, I like keeping this so that we ensure that the configuration can still be parsed with the encoding/json package.

@jskelcy
Copy link
Contributor Author

jskelcy commented Apr 2, 2020

Realizing now that while you suggested the glob approach to backwards compatibility, I went with a different approach but did not explicitly say what it is.

I used a custom unmarshaler that accepts both a list or a single value. I suppose this is more flexible then the glob pattern because you could have SQL files anywhere and pull them in, but in reality I dont know if that is a need anyone has. If we dont think this is useful then I an implement the glob version, but I figured it is worth discussing.

@jskelcy
Copy link
Contributor Author

jskelcy commented Apr 4, 2020

@kyleconroy Bumping the thread here. I have a backwards-compatible change where I use a custom unmarshaler that allows queries and schema to be a single string or a list. It is a little bit more flexible than using a wildcard, but I might be solving a problem that does not exist for most people. Either way, let me know, I can try the wildcard strategy if you are less into this.

@kyleconroy
Copy link
Collaborator

I was using docker compose a few days and encountered this pattern in the wild. docker-compose.yml has a field named env_file that be a string or a list of strings:

env_file: ./foo.env
---
env_file:
  - ./foo.env
  - ./bar.env

The glob syntax I proposed would not have helped me, so I think this is a good strategy.

@kyleconroy kyleconroy merged commit 325418e into sqlc-dev:master Apr 5, 2020
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.

None yet

2 participants