Implemented externalise config constants pass-down as props pattern #19
Conversation
victorskl
commented
May 4, 2023
victorskl
commented
- Minor refactored and improved arrangement
* Minor refactored and improved arrangement
config/constants.ts
Outdated
| description: 'Registry for OrcaBus Events', | ||
| }, | ||
| eventBusProps: { | ||
| eventBusName: 'OrcaBusMain', |
There was a problem hiding this comment.
I guess that will be used at some point in the stateless config, so probably good to pull out into global const?
There was a problem hiding this comment.
Ah. This is already exported as global const i.e. nested/wrapped under orcaBusStatefulConfig object instance.
So. Elsewhere downstream can call like, pseudocode:
new MyConstruct(..., props: cdk.StackProps & OrcaBusStatelessConfig & OrcaBusStatefulConfig) {
...
let busName = props.eventBusProps.eventBusName
}
Or, you mean; do we wish to have a pure global const string for simplicity?
There was a problem hiding this comment.
Hm... good question...
I thought about high level constant, along the same line as
const regName: string = 'OrcaBusSchemaRegistry';
It is true that the value is available from the stateful stack config....
I thought with a separate high level constant the stateless stack could be independent of the stateful stack or its props structure, but the two usually go hand in hand so a bit of dependency is expected/acceptable I guess.
Can always change down the road....
There was a problem hiding this comment.
Ah, like regName var. Right. I know what you mean. Ok, I will update that to realign them.
There was a problem hiding this comment.
Alright. Update with af853c6
| }, | ||
| databaseProps: { | ||
| clusterIdentifier: 'orcabus-db', | ||
| defaultDatabaseName: 'orcabus', |
There was a problem hiding this comment.
Same as for the bus. I suppose the DB name will be needed as well in the stateless stack?
There was a problem hiding this comment.
Good point! It might be more like db SM that need externalise.. I will investigate
There was a problem hiding this comment.
I will do this with separate PR.
As, there's gonna be a bit involving with how to CDK to spin up Aurora Serverless v2. No L2 construct avail; see thread.
https://umccr.slack.com/archives/C03ABJTSN7J/p1683268082908579
| content: content, | ||
| registryName: cProps.registryName, | ||
| registryName: props.registryName, | ||
| type: 'OpenApi3', |
There was a problem hiding this comment.
Could push this into props as well. Make it optional and default to this value (possible use case for optional config + defaults)
There was a problem hiding this comment.
You mean type: 'OpenApi3'? Sure, happy to update that right away, yes.
There was a problem hiding this comment.
Well, since the schema are stateless and defined outside, so this should probably be configurable.
Or would we want to enforce a particular type?
There was a problem hiding this comment.
Ok no prob; I will update it to externalise it as well.
* Handling var declaration repetition within constants.ts is ok * Added externalise SchemaType config
|
Merging... |
