Skip to content
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

[LLRP] CustomConfig setup bug #86

Closed
ajcasagrande opened this issue Jun 17, 2022 · 3 comments · Fixed by #90
Closed

[LLRP] CustomConfig setup bug #86

ajcasagrande opened this issue Jun 17, 2022 · 3 comments · Fixed by #90
Labels
2-medium priority denoting issues with cross-cutting project impact bug Something isn't working jakarta kamakura

Comments

@ajcasagrande
Copy link
Contributor

ajcasagrande commented Jun 17, 2022

I was looking at this code as an example for how to load the custom config, and noticed that this just doesn't seem right. It needs to be checked further.

Mostly around why it is loading the aliasesConfigKey

if err = app.service.LoadCustomConfig(&app.config, aliasesConfigKey); err != nil {
return errors.Wrap(err, "failed to load custom configuration")
}
if err = app.config.AppCustom.AppSettings.Validate(); err != nil {
return errors.Wrap(err, "failed to validate custom config")
}
if err = app.service.ListenForCustomConfigChanges(&app.config.AppCustom, "AppCustom", app.processConfigUpdates); err != nil {
return errors.Wrap(err, "failed to listen for custom config changes")
}

@lenny-goodell
Copy link
Member

Agreed this incorrect. But it is still working for most part because the passed in key is only used to checking for existence in Consul. It uses the passed in struct to un-marshal the TOML. What we'll see is it always pushes config to Consul, which I just confirmed.

level=INFO ts=2022-06-21T19:26:02.7648941Z app=app-rfid-llrp-inventory source=config.go:229 msg="Checking if custom configuration ('Aliases') exists in Configuration Provider"
level=INFO ts=2022-06-21T19:26:02.7708223Z app=app-rfid-llrp-inventory source=config.go:391 msg="Loaded custom configuration from ./res/configuration.toml"
level=INFO ts=2022-06-21T19:26:02.8095057Z app=app-rfid-llrp-inventory source=config.go:270 msg="Custom Config loaded from file and pushed to Configuration Provider "

The result of this is any changes made to existing custom setting in Consul will be overwritten when the service restarts, :-(
But since Aliases is empty in the TOML file, they don't get overridden. Just the values in [AppCustom.AppSettings]

@lenny-goodell lenny-goodell added this to New Issues in Applications WG via automation Jun 21, 2022
@lenny-goodell lenny-goodell added the bug Something isn't working label Jun 21, 2022
@lenny-goodell
Copy link
Member

@ajcasagrande , the better example of using Custom Configuration is here:
https://github.com/edgexfoundry/app-functions-sdk-go/blob/main/app-service-template/main.go#L74-L98

@lenny-goodell
Copy link
Member

The workaround for this bug is to set this values via Env Overrides such as APPCUSTOM_APPSETTINGS_AGEOUTHOURS=5

@lenny-goodell lenny-goodell added the 2-medium priority denoting issues with cross-cutting project impact label Jun 21, 2022
@lenny-goodell lenny-goodell changed the title Potential bug in CustomConfig CustomConfig setup bug Jun 21, 2022
@lenny-goodell lenny-goodell linked a pull request Jun 22, 2022 that will close this issue
5 tasks
Applications WG automation moved this from New Issues to Done Jun 22, 2022
@lenny-goodell lenny-goodell changed the title CustomConfig setup bug [LLRP] CustomConfig setup bug Jun 27, 2022
@lenny-goodell lenny-goodell added 3-high priority denoting release-blocking issues and removed 3-high priority denoting release-blocking issues labels Jun 27, 2022
@lenny-goodell lenny-goodell added this to To Be Considered for Jakarta patch in Jakarta Issues via automation Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2-medium priority denoting issues with cross-cutting project impact bug Something isn't working jakarta kamakura
Projects
Archived in project
Jakarta Issues
To Be Considered for Jakarta patch
Status: Done in main
Development

Successfully merging a pull request may close this issue.

2 participants