SWS 90 Backend for Kubernetes, Prometheus and Istio #19
Conversation
lucasponce
force-pushed
the
SWS-90-Backend-API-Final
branch
from
464d00b
to
d4e361b
Compare
config/config.go
Outdated
| Server Server ",omitempty" | ||
| Identity security.Identity ",omitempty" | ||
| Server Server ",omitempty" | ||
| PrometheusService string ",omitempty" |
There was a problem hiding this comment.
the way the Go YAML processing/parsing works, the name of the yaml property is all lowercase so this is going to be a running word "prometheusservice" in the yaml file or configmap. You should add an underscore _ between words as just having camel-case isn't enough. If this is named Prometheus_Service, the yaml processor will look for the property named "promehteus_service" which is what we would want to make it easier to read.
There was a problem hiding this comment.
Just like the env var, we should append _Url to this to make it clear its a url (Prometheus_Service_Url)
config/config.go
Outdated
| @@ -18,13 +18,21 @@ const ( | |||
| ENV_IDENTITY_CERT_FILE = "IDENTITY_CERT_FILE" | |||
| ENV_IDENTITY_PRIVATE_KEY_FILE = "IDENTITY_PRIVATE_KEY_FILE" | |||
|
|
|||
| ENV_PROMETHEUS_SERVICE = "PROMETHEUS_SERVICE" | |||
There was a problem hiding this comment.
we should append _URL to the name to make it clear this setting expects a URL (and not, for example, a hostname or a "host:port" formatted value).
ENV_PROMETHEUS_SERVICE_URL = "PROMETHEUS_SERVICE_URL"
deploy/openshift/sws.yaml
Outdated
| @@ -9,6 +9,9 @@ parameters: | |||
| - description: The version of the image to use | |||
| name: IMAGE_VERSION | |||
| value: dev | |||
| - description: The namespace to use | |||
| name: NAMESPACE | |||
| value: default | |||
There was a problem hiding this comment.
default should be istio-system, correct?
There was a problem hiding this comment.
+1 yeah I changed in the makefile but it needs to be changed here.
server/server_test.go
Outdated
| server := NewServer(conf) | ||
| config.Configuration = conf | ||
|
|
||
| server := NewServer() |
There was a problem hiding this comment.
you should continue to pass in the config here rather than rely on global var. I think this will make writing unit and i-tests easier (because then we can create servers with different configurations, right?)
I just hate global variables - makes writing tests harder for one reason.
There was a problem hiding this comment.
you don't like global variables - great.
I don't think it makes sense to propagate a config through 4 or 5 layers of abstraction.
I proposed in my 1st PR a mixed approach, you commented to refactor everything - great.
If tests are executed concurrently we can pass as config but if not I see a contradiction in your comments.
There was a problem hiding this comment.
So, to summarize the discussion.
It could be good to have a config.Get() and config.Set() pair that will set the configuration global var stored in the config package.
| httpServer *http.Server | ||
| } | ||
|
|
||
| // NewServer creates a new server configured with the given settings. | ||
| // Start and Stop it with the corresponding functions. | ||
| func NewServer(conf *config.Config) *Server { | ||
| func NewServer() *Server { |
There was a problem hiding this comment.
consider keeping the config as a field in the Server struct and passing it into the NewServer method - see my other comments. Could make writing tests harder if this is relying on global variables.
There was a problem hiding this comment.
I guess with the config.Get() and config.Set() could be enough and we can maintain the NewServer() without config param in the signature.
Do we have an agreement ?
lucasponce
force-pushed
the
SWS-90-Backend-API-Final
branch
3 times, most recently
from
c5564cb
to
af3c1eb
Compare
- Refactor configuration as global variable to avoid propagate it through several layers - Add configuration details for openshift deployments - Change default namespace/project to colocate with istio-system
lucasponce
force-pushed
the
SWS-90-Backend-API-Final
branch
from
af3c1eb
to
2186da1
Compare
|
@jmazzitelli Ok, comments addressed. |
This depends on #18
and it's a split of #17