-
Notifications
You must be signed in to change notification settings - Fork 90
Option for the operator to control a single namespace #1094
Conversation
@@ -131,6 +132,11 @@ final case class Settings(config: Config) extends Extension { | |||
val flinkEnabled = config.getBoolean(s"$root.flink-enabled") | |||
val sparkEnabled = config.getBoolean(s"$root.spark-enabled") | |||
|
|||
lazy val controlledNamespace: Option[String] = Try(sys.env("CONTROLLED_NAMESPACE")) match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about getting it from config and adding a default in the config lie we do in other places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, but, in this specific case, I decided to use an env variable to make it easier and clean in the helm chart.
e.g. https://github.com/lightbend/cloudflow-helm-charts/pull/23/files#diff-08bec0d8da17f5482f6840b57e144767f8250cafd31b80a37e13e494d2236b71R5
Having the default config would require some additional logic to be implemented there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(the env var could have been used in the reference.conf as ${?VAR}
so it is known what is used, but this is also in the code so it does not really matter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I guess I'm thinking of this as, the reference.conf tells me what can be configured, or which env props can be used to change things, but this is a minor detail)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree that would be nicer, I'll bind it to the operator configuration 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RayRoestenburg done here:
c7e5386
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
With: lightbend/cloudflow-helm-charts#23