Updates codebase to Go 1.14, preps for Heroku deploy #10
Conversation
* Corrects use of non-pointer as second argument in server_test. * Uses uid and pid consistently.
* Adds go.mod, go.sum * Adds Procfile * Fixes GitHub Actions workflow
| // OpenFromCreds returns a pointer to a database client based on | ||
| // JSON credentials pointed to by the provided path. | ||
| // OpenFromEnv returns a pointer to a database client based on | ||
| // JSON credentials given by the environment variable. | ||
| // Returns an error if it fails at any point. | ||
| func OpenFromCreds(ctx context.Context, path string) (*DB, error) { | ||
| // check, using os.Stat(), that the file exists. If it does not exist, | ||
| // then fail. | ||
| if _, err := os.Stat(path); err != nil { | ||
| return nil, err | ||
| func OpenFromEnv(ctx context.Context) (*DB, error) { | ||
| cfg := os.Getenv("TLACFG") | ||
| if cfg == "" { | ||
| return nil, fmt.Errorf("no $TLACFG environment variable provided") | ||
| } | ||
|
|
||
| // set up the app through which our client will be | ||
| // acquired. | ||
| opt := option.WithCredentialsFile(path) | ||
| opt := option.WithCredentialsJSON([]byte(cfg)) | ||
| app, err := firebase.NewApp(ctx, nil, opt) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Updates OpenFromEnv to use the TLACFG envvar.
| // check for PORT variable. | ||
| port := os.Getenv("PORT") | ||
| if port == "" { | ||
| log.Printf("no $PORT environment variable provided, defaulting to '%s'", DEFAULTPORT) | ||
| port = "8081" | ||
| } | ||
|
|
There was a problem hiding this comment.
Gets port to serve on from envvars; defaults to 8081 if not provided.
|
Thanks Tomo! Will be merging now then and moving on to prepping our first release. |
@krashanoff Why is this a good thing? Doesn't it make it a bit more annoying to test when running |
|
Should there be a note that to run tests, this line must be ran first? export TLACFG="$(cat credentials.json)" |
|
Hi Michael,
Your point about "dotenv-esque tooling" is close to the mark. If I remember correctly, these tests required a Firebase connection to run (notice `Db` is not an interface type). To make them work in CI, I think we had saved the test environment's credentials into the repo's secrets. Your note about `export TLACFG=...` is correct. When I was working on this, I just copied the contents of the credentials file into my `.env`, delimited by single quotes so bash would handle the JSON nicely.
I agree that use of a `TLACFG` variable holding JSON credentials makes running and testing cumbersome. If I had to write the changes described here again, I'd probably just shove everything into a single config file or pull straight from the `credentials.json` itself, using the environment as a fallback.
I'd also recommend lifting database functionality to an interface and ensure tests run with a mock. Then, if the credentials field in the application's config is missing in a development environment, the program can switch to a degraded mode of operation using the mock.
Happy to answer other questions, but my contributions are over three years old now and have not crossed my mind in some time. Out of curiosity, what has got you looking into these old changes?
Leo
|
|
Gotcha! There's good news though: other maintainers (particularly Timothy to my knowledge) have been working on this exact issue. There is already an interface (
I'm the new maintainer, as Timothy, the previous maintainer, has graduated! This past quarter I've been familiarizing myself with the frontend codebase, but now that I have a pretty good mental model of everything going on in it, I thought I'd dig into the backend code too. My plans for the near(ish) future are:
|
|
Michael,
In my opinion, a mixture of two would be best, but frontloading more tests to use the mock seems like a better investment. I think Firebase is pretty straightforward, so any logic errors will be easy to catch ad-hoc if you use it while developing new features locally. Of course, writing proper E2E tests would be perfect, but it's all in the balance.
Glad that you're taking the initiative as the new maintainer. Your short-term plans sound well-scoped. Feel free to reach out if you have questions about the project's early history, and I can do my best to answer them.
Leo
|
Big PR round two. This is a much more digestible PR -- though it does still touch a great number of files in the repo. Here are all the changes made:
$TLACFGenvironment variable, and set its port from the$PORTvariable.