Merged
Conversation
fgsch
reviewed
Apr 15, 2021
Contributor
Author
|
The implementation currently doesn't work. Discussed with Kailan and am addressing a different way. |
Contributor
Author
fgsch
reviewed
Apr 15, 2021
fgsch
reviewed
Apr 15, 2021
kailan
approved these changes
Apr 16, 2021
Member
kailan
left a comment
There was a problem hiding this comment.
Code reviewed and tested locally – works as expected on configurations from 0.26.x. No more requests than needed are made to the config service. Ship it 🚀
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem: When we have an updated configuration the CLI code is updated so it can parse the new configuration schema, but if the CLI is run (this initial run will cause the config data to be cached for 5mins) before the binary is updated to a new version (which includes code that expects new config fields) and if the new CLI version is then subsequently run before the currently cached config TTL has expired, then a user will find the new CLI code will try to read a struct field that doesn't have a value because the underlying configuration data isn't updated.
See also: #258
Solution: The customer waits 5 minutes (or however long the TTL is set to in the config) and then tries the CLI command again and the configuration will be considered stale and subsequently updated before the subcommand is executed.
Better Solution: Implement a check alongside the 'stale config check' that validates if the CLI binary was recently modified (i.e. updated) and if so we'll force the fetching of the configuration.
Notes: this will require an update to the configuration endpoint to include a new
version.