volatiletech / sqlboiler

Generate a Go ORM tailored to your database schema.
BSD 3-Clause "New" or "Revised" License
6.66k stars 539 forks source link

Encoding outputDirDepth in boil_main_test.go makes running sqlboiler output vary across machines for the same schema #476

Open autarch opened 5 years ago

autarch commented 5 years ago

If you're having a generation problem please answer these questions before submitting your issue. Thanks!

What version of SQLBoiler are you using (sqlboiler --version)?

SQLBoiler v3.2.0 (but with my PRs merged in)

If this happened at generation time what was the full SQLBoiler command you used to generate your models? (if not applicable leave blank)

The usual.

If this happened at runtime what code produced the issue? (if not applicable leave blank)

What is the output of the command above with the -d flag added to it? (Provided you are comfortable sharing this, it contains a blueprint of your schema)

Please provide a relevant database schema so we can replicate your issue (Provided you are comfortable sharing this)

This isn't schema-related.

Further information. What did you do, what did you expect?

So the issue is that encoding outputDirDepth in the boil_main_test.go generated file causes this file to vary when run across different machines, which is annoying if you check in your generated code.

But what's really confusing is why this exists at all. The outputDirDepth is calculated in a way that's always guaranteed to give end up at my machine's root directory (on Windows I guess it'd be the drive root dir). I don't know why you would expect to find a sqlboiler config file in the root.

I don't see any reason why this bit of code shouldn't be removed. If you really want to check the root dir there are easier ways to get that value.

aarondl commented 5 years ago

The idea is that sqlboiler is always run from a project directory like /home/user/project, and that there is an sqlboiler.toml configuration file in there. We expect the models to be generated and stored at /home/user/project/models though it can be named whatever we want and be as many folders deep in the project root as we want but the idea is that it's always generated from the same directory and tested from said root directory.

If we don't do this, then we have to copy the config file around to the generated tests which is painful as the test directory is recommended to be COMPLETELY eradicated every time we regenerate models. Having copies of a configuration file is also obviously bad since you then need to update multiple files if you do change your database configuration.

Currently I can't see how this would ever find your Drive root directory unless there's a bug or you're generating models and putting them in C:\models. Which of course sqlboiler was created in the days before Go modules so this would have been completely odd but now with go modules it could be considered slightly less odd? Could you perhaps elaborate on the use case you have so I can understand better why the code isn't working as intended?

Perhaps one solution is to allow the config file for tests to be specified on the command line to avoid the problems you're running into.

I'm also okay with suggestions for better methods of determining the root directory.

autarch commented 5 years ago

I think there's two issues.

One, I think there might be better ways to find this config file that don't require encoding information about the models path into the generated code.

Two, the way it calculates output dir depth has some issues. I realized that my case is a bit odd because the config file has a fully qualified path rather than a relative path, so I end up with something like models = "/home/autarch/go/src/github.com/OrgName/project/path/to/models" in my config. This is why the calculation for the output dir leads back to my filesystem root.

This happens because I have a script to generate the config. Why do that? Well, the config requires things like a Pg username and password that could vary between different places where the script is run, so it's a lot easier to generate the config, stick in a temp dir, and then run sqlboiler (but now I'm looking at this and realizing that with viper I could just use env vars instead of a temp file).

So there's nothing preventing someone from putting a fully qualified path in output in the config, and in fact this seems to work, except for the way it tries to calculate the location of the config file. There's also nothing preventing someone from not having a config file at all.

I think trying to reuse a config file from tests seems like it just won't work very well, because you'd have to have an identical config on developer machines and in CI environments. It seems like that's unlikely to work for many cases.

Of course, you can also use env vars for running the tests.

So I'm not really sure what the takeaway is here. I guess I'd suggest a few things:

  1. Find a better way to look for a config file. The simplest thing would be to just add an env var that is checked in boil_main_test.go and tell people to use that rather than trying to figure out the config file's dir at runtime. That way the content of boil_main_test.go becomes 100% repeatable.

1.a. The less simple way would be to walk up the directory tree from the models dir looking for the config file. There's a risk you'd find an incorrect one somehow (maybe in a case of using submodules?) but it's likely to work for most cases.

  1. Document the fact that you don't need a config file at all and encourage people to go that route longer term. It can be a little harder to get started with that approach it is a lot more flexible in allowing for variation between environments where this code might be run. The README.md does mention env vars but it doesn't give any examples, and the env var names are not obvious (plus ideally they'd be prefixed with SQLBOILER_).

(As an aside, SQLBoiler could really use its own doc site as opposed to just a README.md to better organize that information. Please check your mail for the shipment of round tuits I've ordered for you ;)

aarondl commented 5 years ago

Missed replying to this apparently. I think we're fine to have an overriding environment variable AND a walk up the directory tree and discard this whole depth thing. Fine idea.

And for what it's worth I disagree with the doc site but it's because I don't understand what benefits it brings over a README given that you can basically write whatever HTML you want in the readme, it has a table of contents full of links, and is in an obvious and central spot. Maybe you could elaborate on why it'd be helpful