add override #331
add override #331
Conversation
k3llymariee
commented
Jun 25, 2024
•
k3llymariee
commented
internal/dev_server/api/server.go
Outdated
| panic("implement me") | ||
| store := model.StoreFromContext(ctx) | ||
| err := store.DeleteOverride(ctx, request.ProjectKey, request.FlagKey) | ||
| return DeleteDevProjectsProjectKeyOverridesFlagKey200JSONResponse{}, err |
There was a problem hiding this comment.
not filling out this response body - it'll just default to false/null anyway which feels ok?
There was a problem hiding this comment.
Maybe a 204 would be better if you're expecting an empty response?
There was a problem hiding this comment.
We should either 204 or keep it 200 & return like {Override: false, Value:...}.
| @@ -162,13 +162,13 @@ func NewAddProjectCmd(client dev_server.LocalClient) *cobra.Command { | |||
| //} | |||
|
|
|||
| type postBody struct { | |||
| sourceEnvironmentKey string | |||
| SourceEnvironmentKey string `json:"sourceEnvironmentKey"` | |||
| @@ -2,3 +2,4 @@ | |||
| dist/ | |||
| *.log | |||
| node_modules/ | |||
| devserver.db | |||
internal/dev_server/db/sqlite.go
Outdated
|
|
||
| func (s Sqlite) DeleteOverride(ctx context.Context, projectKey, flagKey string) error { | ||
| _, err := s.database.Exec(` | ||
| UPDATE overrides set active = false where project_key = ? and flag_key = ? |
There was a problem hiding this comment.
This should also increment the version. This is needed so that the sdk will pick up the change (it ignores changes if the version number isn't bigger).
There was a problem hiding this comment.
When upserting we probably want to check the flag exists in the flagState for the project if not there will just be bogus entries if someone mistypes a flag name.
Also a bonus would be to make sure the types of the flag values match but I know that's kinda a lot of work so maybe this can be done if we have time.
I'm not clear on how we are going to override values for boolean flags. I tried setting the body to { "enable-deployment-link": "false"} but it stored this entire thing in the value column which I don't think we want. That said, I'm sure this is just me not understanding what the api expects.
This seems fine enough to merge for now and build off of if you'd rather add the validation check in a separate pr.
