Skip to content

Commit

Permalink
Fix type validation in configstore DC client value updating (cadence-…
Browse files Browse the repository at this point in the history
…workflow#5110)

* Remove misleading type check, Add more detailed log message

* removing debugging logging

* Handle nil update edge case

---------

Co-authored-by: allenchen2244 <[email protected]>
Co-authored-by: Zijian <[email protected]>
  • Loading branch information
3 people authored Mar 17, 2023
1 parent a3e2774 commit 1519ace
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 2 deletions.
2 changes: 1 addition & 1 deletion common/dynamicconfig/configstore/config_store_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ func (csc *configStoreClient) GetListValue(name dc.ListKey, filters map[dc.Filte

func (csc *configStoreClient) UpdateValue(name dc.Key, value interface{}) error {
dcValues, ok := value.([]*types.DynamicConfigValue)
if !ok && dcValues != nil {
if !ok && value != nil {
return errors.New("invalid value")
}
return csc.updateValue(name, dcValues, csc.config.UpdateRetryAttempts)
Expand Down
2 changes: 1 addition & 1 deletion service/frontend/adminHandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ func (adh *adminHandlerImpl) AddSearchAttribute(
// update dynamic config. Until the DB based dynamic config is implemented, we shouldn't fail the updating.
err = adh.params.DynamicConfig.UpdateValue(dc.ValidSearchAttributes, currentValidAttr)
if err != nil {
adh.GetLogger().Warn("Failed to update dynamicconfig. This is only useful in local dev environment. Please ignore this warn if this is in a real Cluster, because you dynamicconfig MUST be updated separately")
adh.GetLogger().Warn("Failed to update dynamicconfig. This is only useful in local dev environment for filebased config. Please ignore this warn if this is in a real Cluster, because your filebased dynamicconfig MUST be updated separately. Configstore dynamic config will also require separate updating via the CLI.")
}

// update elasticsearch mapping, new added field will not be able to remove or update
Expand Down

0 comments on commit 1519ace

Please sign in to comment.