-
Notifications
You must be signed in to change notification settings - Fork 32
feat(POC): add run time config management service #2355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat(POC): add run time config management service #2355
Conversation
e8b20cd to
26bb728
Compare
…inals for sync change logging
6a92f73 to
e7e05c5
Compare
|
@Junjiequan Can you add an example for how to use this? |
bpedersen2
left a comment
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.
Sound good in general.
I guess a filter for certain fields will be needed in the end if we allow be-config changes, e.g. to avoid loss of all admin groups or changes to mongodb settings( I would probably keep all security-related fields as file-only).
Frontend-config should be less critical.
sbliven
left a comment
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 think we should discuss the core premise of this POC, which is that the configuration should be stored in the database instead of in files. While it would be convenient to be able to change the configuration on the fly, I'm worried that it will become even harder to track configuration changes. If we do it, I think there should be a clear process to "burn in" the configuration: export non-default settings to an env file, empty the database, and restart the backend with a clean configuration. Otherwise it's hard to keep track of changes.
Can you add a description of the usage to the README or docs? Even if it changes later, it would be helpful to read how the merging works. I think you said that the database is considered canon and overrides anything in the config files or the defaults.
How does this fit with /admin/config? Would this provide the merged "frontend" data?
Likewise, will calls to the configService be merged to the "backend" data? If so, we should double-check that there aren't any remaining direct calls to configuration(), as those would bypass the configService.
I guess you would add more endpoints
- get the full, merged configuration
- get/set individual properties (or would this just be done with PATCH application/json+merge?)
- way to delete configuration changes (resetting from the files or defaults)
| export class RuntimeConfigController { | ||
| constructor(private readonly runtimeConfigService: RuntimeConfigService) {} | ||
|
|
||
| @AllowAny() |
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.
Needs the swagger descriptors (ApiOperation, ApiParam, etc)
| example: "frontend", | ||
| }) | ||
| @IsString() | ||
| _id: string; |
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 suggest key for the unique identifier. _id should only be used for the internal mongo document id (which should not be accessible in the API).
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.
Does it make sense to have more than "frontend" and "backend"? I would make this an enum. Or are you thinking there might be use cases where other microservices store most of their configs on the scicat backend?
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.
_id should only be used for the internal mongo document id (which should not be accessible in the API).
I'm not sure about this statement.
await this.datablocksService.findOne({ where: { _id: id }, }); we have been implementing query like this in many places, I don't see what's the cons here..
Does it make sense to have more than "frontend" and "backend"? I would make this an enum. Or are you thinking there might be use cases where other microservices store most of their configs on the scicat backend?
We don’t want to lock it to just “frontend” or “backend”.
The idea is to let operators store any JSON config they want, and the backend shouldnt restrict it. They decide what config files they want to use, not us. It could be frontend-config, published-data-schema.json, proposal-ui-config etc
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 would suggest just id as we ill store the whole configuration in one document... or at least, that was the idea last time we brainstormed.
| }) | ||
| @IsOptional() | ||
| @IsString() | ||
| description?: string; |
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's the use of this? Just internal documentation?
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.
That's the idea.
If a site admin uses this subsystem to store additional configurations, we should also have a name and a description for management purposes.
| @@ -0,0 +1,42 @@ | |||
| export const isPlainObject = ( | |||
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.
Use common.utils.IsRecord()
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.
Agreed
| constructor(private readonly runtimeConfigService: RuntimeConfigService) {} | ||
|
|
||
| @AllowAny() | ||
| @Get("data/:id") |
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.
data strikes me as a surprising choice for the endpoint path. I see a close link between this functionality and the /admin/config endpoint. Maybe it could just be GET/PUT/PATCH /admin/config/:id? And there should be a way to get either the merged configuration or just the overridden values.
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 do not remember why we decided to move it away from admin/config.
If I remember correctly, the reason for it is that this subsystem can become a store for additional blobs besides configurations. In our mind admin/config relates only to admin users, which is true for updating but not for retrieving.
Hopefully it makes sense, but I will be happy to discuss further
We defintelly needs to discuss more about this, I just wanted to pull the idea into code, so we can have something to disccuss upon. If i understand your proposal correctly, you are suggesting that:
So, the config file is the source of truth, config record is temporary copy for real time editing. once the backend restarts the temporary state resets and rebuilt from the file again. Is it correct intepretation?
Sure, I will provide it as soon as possible.
It's up for discussion that wheter if we still want to keep /admin/config after we have the runtimeConfig endpoint.
The backend data ( environment variables I assume ) is completly seperated logic and should not be relevant to the run time config service at all, they are out of scope this time at least. The run time service serves for frontend related configs only.
I imagine the entry sturcture will look like this: [
{
"_id": "frontendConfig",
"description": "Some text to be rendered on the UI",
"data": {
"...": "raw JSON blob"
}
},
{
"_id": "frontendTheme",
"data": {
"...": "raw JSON blob"
}
},
{
"_id": "someOtherConfig",
"data": {
"...": "raw JSON blob"
}
}
]On the frontend we use |
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 would also the fields createdBy, updatedAt, createdAt.
| constructor(private readonly runtimeConfigService: RuntimeConfigService) {} | ||
|
|
||
| @AllowAny() | ||
| @Get("data/:id") |
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 do not remember why we decided to move it away from admin/config.
If I remember correctly, the reason for it is that this subsystem can become a store for additional blobs besides configurations. In our mind admin/config relates only to admin users, which is true for updating but not for retrieving.
Hopefully it makes sense, but I will be happy to discuss further
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 naming is perfect if we are going to use this subsystem to store only configurations.
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.
Same as my previous comment
| @@ -0,0 +1,42 @@ | |||
| export const isPlainObject = ( | |||
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.
Agreed
|
@sbliven valid feedback!!! |
Description
Motivation
Fixes
Changes:
Tests included
Documentation
official documentation info