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

[Configuration] New APIs to simplify pull and updates #101

Open
lenny-goodell opened this issue Jul 12, 2023 · 2 comments
Open

[Configuration] New APIs to simplify pull and updates #101

lenny-goodell opened this issue Jul 12, 2023 · 2 comments
Labels
enhancement New feature or request hold Intended for PRs we want to flag for ongoing review

Comments

@lenny-goodell
Copy link
Member

lenny-goodell commented Jul 12, 2023

🚀 Tech Debt

Relevant Package [REQUIRED]

This feature request is for APIs to pull configuration and receive updated configuration (i.e. `Writable`) and the Consul implementation.

Per Napa planning this will not be included due to risk of destabilizing the release. Task for this release is to agree on the approach and possibly create a POC.

Description [REQUIRED]

The current APIs expect the service's complete config struct to be passed in when pulling config and waiting for updates. The Consul implementation uses `github.com/mitchellh/consulstructure` in both cases.

There are two issues with this approach:

  1. github.com/mitchellh/consulstructure doesn't support pull/marshaling slices.
  2. github.com/mitchellh/consulstructure updates the passed in service's config struct with the complete Writable section when a setting changes and doesn't provide indication of which settings have changed. With the need to merge configurations for the recent Common Config implementation, this has lead to this issue when map items are removed.

Describe the solution you'd like

  • Add new APIs (deprecate old versions) for pulling complete configuration and waiting for updates.
    • GetConfigurationMap() {map[string]any, error)
      This new API returns the service's complete configuration as a map which the caller can then use to merge/marshal into the service's config struct.
    • WatchForUpdates(updateChannel chan<- map[string]Update, errorChannel chan<- error, watchKey string)
      This new API will watch for updates, post a map containing only the fields that have changed. The setting paths are the map keys and the map values contain the new setting value and if the setting is existing, new or deleted (for handling of maps/slices).
  • Consul implementation would no longer use github.com/mitchellh/consulstructure. Instead would implement pulling/waiting internally. Long request on watchKey url for Updates and simple pull on root key for Get.

go-mod-bootstrap can then be refactored to use these new APIs to resolve edgexfoundry/go-mod-bootstrap#534 and enable slice as a type for configuration items.

Describe alternatives you've considered

Have you considered any alternative solutions or workarounds?
@lenny-goodell lenny-goodell added the enhancement New feature or request label Jul 12, 2023
@lenny-goodell lenny-goodell changed the title [Configuration] Redesign API and Consul implementation to simplify updates from Consul [Configuration] Redesign API and Consul implementation to simplify pull and updates Jul 12, 2023
@lenny-goodell lenny-goodell changed the title [Configuration] Redesign API and Consul implementation to simplify pull and updates [Configuration] New APIs and Consul implementation to simplify pull and updates Jul 12, 2023
@lenny-goodell lenny-goodell changed the title [Configuration] New APIs and Consul implementation to simplify pull and updates [Configuration] New APIs to simplify pull and updates Jul 12, 2023
@github-project-automation github-project-automation bot moved this to New Issues in Technical WG Jul 12, 2023
@lenny-goodell lenny-goodell added the hold Intended for PRs we want to flag for ongoing review label Jul 12, 2023
@lenny-goodell
Copy link
Member Author

@cloudxxx8 , please review and comment.

@cloudxxx8
Copy link
Member

@lenny-intel thanks for the reminder. I've reviewed the issue description, and the design looks good to me.
Is it the typo in the signature?
updateChannel chan<- map[string]Update
I believe it should be
updateChannel chan<- map[string]any
Or, would you like to define a struct called Update?

@jumpingliu jumpingliu moved this from New Issues to Icebox in Technical WG Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hold Intended for PRs we want to flag for ongoing review
Projects
Status: Icebox
Development

No branches or pull requests

2 participants