Add a global configuration #444
Conversation
t-persson
requested review from
andmat900 and
fredjn
and removed request for
a team
| package config | ||
|
|
||
| // Config holds common configuration options for the controllers. | ||
| type Config struct { |
There was a problem hiding this comment.
The name config.Config seems to broad. I understand, technically it does not conflct with anything. I am referring to the name itself. In cluster_types.go many names are more specific, such as ETOSSuiteStarterConfig.
There was a problem hiding this comment.
The reason for the ETOSSuiteStarterConfig in cluster_types.go is because there are a lot of types there that share namespace.
What other name than config.Config do you suggest in this case?
There was a problem hiding this comment.
There is a lot of config "going" around 😄 Ex. config vs r.config. It's not totally unreasonable that this could cause confusion in the future, but I guess it's not a deal braker (or a thing to fix) for this PR.
| } | ||
|
|
||
| eiffelbus := extras.NewRabbitMQDeployment(cluster.Spec.MessageBus.EiffelMessageBus, r.Scheme, r.Client) | ||
| eiffelbus := extras.NewRabbitMQDeployment(cluster.Spec.MessageBus.EiffelMessageBus, r.Scheme, r.Client, r.Config) |
There was a problem hiding this comment.
Is there a need to have r.Config? Cannot it be handled via cluster.spec which is a global configuration too?
There was a problem hiding this comment.
Not everything in this config is exposed via the cluster specification.
There was a problem hiding this comment.
Correction: Not everything in this config should need to be exposed in the cluster speficiation
There was a problem hiding this comment.
How do we balance this, what is the rule to follow here?
There was a problem hiding this comment.
The rule is, if we don't want users to modify it then it should be in this configuration. If we want users to modify it then it should be in the cluster spec.
I don't know know why we added default hosts to this configuration, but the reason it was created was for the etcd image: #390 (comment) which has not been added for some reason.
There was a problem hiding this comment.
I can remove this PR if this is no longer a change we want
| } | ||
| etcdClientPort, err := strconv.Atoi(r.config.Database.DefaultPort) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
This is unlikely to fail, however it might still be a good idea wrap this error with some context to help in case of misconfiguration.
| package config | ||
|
|
||
| // Config holds common configuration options for the controllers. | ||
| type Config struct { |
There was a problem hiding this comment.
There is a lot of config "going" around 😄 Ex. config vs r.config. It's not totally unreasonable that this could cause confusion in the future, but I guess it's not a deal braker (or a thing to fix) for this PR.
3 participants
Applicable Issues
Split commit from: #382
Description of the Change
Create a global configuration for default parameter values.
Alternate Designs
Possible Drawbacks
Sign-off
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.
Signed-off-by: Tobias Persson [email protected]