Scaffold Respository#1
Conversation
terryyylim
requested review from
krithika369 and
pradithya
and removed request for
krithika369 and
pradithya
|
cc: @pradithya |
| make setup | ||
| ``` | ||
|
|
||
| 3. On push, the pre-commit hook will run. This runs `make format`, `make lint`, `UI linting` and `generation of helm docs`. |
There was a problem hiding this comment.
There is no UI linting / helm doc gen (at least, not yet). Shall we remove it?
| return nil | ||
| } | ||
|
|
||
| func reflectViperConfig(prefix string, spec interface{}, v *viper.Viper) error { |
There was a problem hiding this comment.
A docstring for the function describing what it does would be useful.
| @@ -0,0 +1,95 @@ | |||
| package config | |||
There was a problem hiding this comment.
Why do we have this common module already? Is it expected that this repo will contain the dataset service and other services later on as well?
| ) | ||
|
|
||
| type Config struct { | ||
| HTTPPort int `envconfig:"CARAML_HTTP_PORT" default:"8081"` |
There was a problem hiding this comment.
Curious, why the choice to prefix the envvar name with CARAML_ as opposed to something like APP_ which seems to be our convention in Turing at least (example).
There was a problem hiding this comment.
No reason, I just thought perhaps we can consolidate the ports to be similar for all our services in CaraML ecosystem.
|
|
||
| // Create gRPC server | ||
| srv := &Server{ | ||
| observationClient: observationClient, |
There was a problem hiding this comment.
Why do we need the gRPC client in the server? Thought we'd instead be calling this.
There was a problem hiding this comment.
Yeah good point, we don't need it, removed!
terryyylim
force-pushed
the
spike-observation-service
branch
from
5dbf098 to
1090e3d
Compare
| @@ -0,0 +1,30 @@ | |||
| module github.com/caraml-dev/observation-service/common | |||
|
|
|||
| go 1.18 | |||
There was a problem hiding this comment.
Since the repository started with multi module implementation, can we evaluate using workspace ?
terryyylim
force-pushed
the
spike-observation-service
branch
from
d64c148 to
1090e3d
Compare
3 participants
What this PR does / why we need it:
This PR scaffolds the repository with the following:
Screen.Recording.2022-11-22.at.1.24.54.PM.mov
Which issue(s) this PR fixes:
None