Scaffold Respository #1
Conversation
terryyylim
requested review from
krithika369 and
pradithya
and removed request for
krithika369 and
pradithya
|
cc: @pradithya |
CONTRIBUTING.md
Outdated
| 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?
observation-service/config/config.go
Outdated
| ) | ||
|
|
||
| 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.
observation-service/server/server.go
Outdated
|
|
||
| // 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
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