Fetch dev tree from GitHub #331
Conversation
web/src/main/go/internal/utils.go
Outdated
| func GetTreeFilepath(name string) string { | ||
| env := os.Getenv("ENV") | ||
|
|
||
| if name == DEV_TREE_IDENTIFIER+".json" && env == "production" { |
There was a problem hiding this comment.
@bprize15 Please suggest if this is the best way to identify production deployment.
There was a problem hiding this comment.
Setting APP_ENV=production in the Dockerfile
web/src/main/go/internal/utils.go
Outdated
| func fetchDevTreeIfChanged(devTreePath string) error { | ||
| etagPath := devTreePath + ".etag" | ||
|
|
||
| req, err := http.NewRequest("GET", DEV_TREE_GITHUB_RAW_URL, nil) |
There was a problem hiding this comment.
Can you use http.MethodGet instead of the string literal "GET"
| return err | ||
| } | ||
|
|
||
| if etag := resp.Header.Get("ETag"); etag != "" { |
There was a problem hiding this comment.
Could we save the etag at a different path? I think we should maintain that everything of the trees dir can be expected to be a tree
web/src/main/go/internal/utils.go
Outdated
|
|
||
| if name == DEV_TREE_IDENTIFIER+".json" && appEnv == "production" { | ||
| devTreePath := filepath.Join(TREE_FILES_PATH, DEV_TREE_IDENTIFIER+".json") | ||
| _ = fetchDevTreeIfChanged(devTreePath) |
There was a problem hiding this comment.
I think we should handle this error. Also, I think we should handle fetching the dev tree somewhere else. It feels confusing that this function also fetches the dev tree, and if we handle the error here, we force any call to GetTreeFilepath to handle this error.
I think a better spot might be ReadTreeFromFile. Since this already returns an error, you won't need to add extra error handling (probably), and reading the tree seems more likely to have the side effect of making sure the dev tree is up to date than getting a filepath. This is just one way of fixing it, so feel free to implement differently if you have a better solution.
web/src/main/go/internal/utils.go
Outdated
| return nil // already up-to-date | ||
|
|
||
| case http.StatusOK: | ||
| tmp := devTreePath + ".tmp" |
There was a problem hiding this comment.
Could we do this work somewhere else than the root of the tree repo?
web/src/main/go/internal/utils.go
Outdated
| } | ||
|
|
||
| if etag := resp.Header.Get("ETag"); etag != "" { | ||
| _ = os.WriteFile(etagPath, []byte(etag), 0644) |
There was a problem hiding this comment.
I don't think we should get in the habit of ignoring errors. Callers of this function would have no idea the etag writing can fail. This won't cause any errors in this case, but generally the caller should be aware of what can go wrong so it can decide how to handle it. Maybe we should add a lint rule to enforce this.
web/src/main/go/internal/utils.go
Outdated
| ) | ||
|
|
||
| func ReadTreeFromFile(name string) (Tree, error) { | ||
| if err := maybeUpdateDevTree(name); err != nil { |
There was a problem hiding this comment.
I think this function name is a bit confusing. I would try to be more descriptive. My intuition is just to move all the logic from maybeUpdateDevTree to readTreeFromFile (since it is only used once and I find it easier to read without jumping around).
web/src/main/go/internal/utils.go
Outdated
| func ReadTreeFromFile(name string) (Tree, error) { | ||
| if err := maybeUpdateDevTree(name); err != nil { | ||
| return nil, err | ||
| appEnv := os.Getenv("APP_ENV") |
There was a problem hiding this comment.
Since this is the same logic as ReadTreeRaw let's reuse that here
Uh oh!
There was an error while loading. Please reload this page.