Nats kv config #156
Nats kv config #156
Conversation
joelrebel
commented
Sep 5, 2023
joelrebel
commented
- Fixes a bug - create consumer when it doesn't exist, instead of returning an error
- Declare NATS kv config parameter
instead of returning the error
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #156 +/- ##
==========================================
- Coverage 66.99% 66.92% -0.07%
==========================================
Files 17 17
Lines 1027 1028 +1
==========================================
Hits 688 688
- Misses 286 287 +1
Partials 53 53
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
events/nats_config.go
Outdated
| @@ -66,6 +66,14 @@ type NatsOptions struct { | |||
|
|
|||
| // Setting Stream parameters will cause a NATS stream to be added. | |||
| Stream *NatsStreamOptions `mapstructure:"stream"` | |||
|
|
|||
| // Nats KV parameters | |||
| KV *NatsKVOptions `mapstructure:"kv"` | |||
There was a problem hiding this comment.
mainly for consistency in loading the NATS config from the env/config file
https://github.com/metal-toolbox/sandbox/pull/26/files#diff-4d55b7b2bd04c8f271ca9fb638669a82d8d00aba55ccc6bde46aafd0c72e9236R32
Also I'd like to remove the --replica-count cli flag since its an internal thing and I don't want all controllers to have to specify this through the cli - https://github.com/metal-toolbox/flasher/blob/aa6c0eb71ddafea5878008d0ad8c193c75580464/cmd/run.go#L119
There was a problem hiding this comment.
The only reason we'd change replica count is to support running in the sandbox or in other integration tests. I don't think this should be configurable.
events/nats_config.go
Outdated
| @@ -66,6 +66,14 @@ type NatsOptions struct { | |||
|
|
|||
| // Setting Stream parameters will cause a NATS stream to be added. | |||
| Stream *NatsStreamOptions `mapstructure:"stream"` | |||
|
|
|||
| // Nats KV parameters | |||
| KV *NatsKVOptions `mapstructure:"kv"` | |||
There was a problem hiding this comment.
The only reason we'd change replica count is to support running in the sandbox or in other integration tests. I don't think this should be configurable.
|
Ack, how do you suggest we proceed here. I would prefer if it wasn't a CLI parameter since a user of flasher/other controller should not have to care about this detail, and if we want to keep all the NATs options in one place, it made sense to be here - so its managed in one place. |
This feels like we’re supporting a mostly-unnecessary code path. Replica count isn’t a property of Alloy (or flasher or condition-orchestrator), it’s a property of the KVs they connect to, and the workers need to know about it in the case that they need to create the nats infrastructure on a fresh install. If we remove that responsibility from the workers, we can instead have the workers fail if they can’t reach a NATS consumer or KV that is required. Then we put all NATS setup in code that runs prior to the workers being deployed. Thus we’re covered for a “new site turn-up” scenario where NATS is only freshly available, and we remove complexity and extraneous detail from the controllers. |
|
Ack, to make sure I understand this correctly - we ditch all the NATS JS/KV/Consumer init configuration parameters and code from all the controllers for a single controller that is responsible for the NATS init, and has all the parameters in code and not in a configuration file? I'm curious if this is going to work out well if we want to roll out changes to these parameters quickly, given how involved a code change + deployment to k8s is.. To be clear - Im on board with idea of a controller/cronjob ensuring the configuration parameters in question, Im not totally convinced having all the JS/KV/Consumer parameters in code will allow us to be flexible. I'm fine with taking that approach and seeing where it leads us though. |
This reverts commit 16cb09f.
|
reverted kv config param commit based on slack discussion |
