feat(datafile-management): Introducing config manager for managing project config - WIP #175
Conversation
| # Implementation of ProjectConfigManager interface. | ||
| # Returns the stored ProjectConfig instance. | ||
| | ||
| def initialize(datafile = nil, logger = nil, error_handler = nil, skip_json_validation = false) |
There was a problem hiding this comment.
Fallback projectconfigmanager should only take ProjectConfig. to create DatafileProjectConfig from datafile must be in any utils method.
| | ||
| private | ||
| | ||
| def set_config(datafile) |
There was a problem hiding this comment.
we don't really need it. Make it simple.
| | ||
| def running | ||
| # Check if polling thread is alive or not. | ||
| @polling_thread.alive? |
There was a problem hiding this comment.
Where is auto_update?
| | ||
| def running | ||
| # Check if polling thread is alive or not. | ||
| @polling_thread.alive? |
There was a problem hiding this comment.
where stopping polling_thread
| # Triggered as part of the thread which fetches the datafile and sleeps until next update interval. | ||
| while running | ||
| fetch_datafile | ||
| sleep @update_interval |
There was a problem hiding this comment.
Please add a check if we don't get response before our next schduled call, then no need to fetch again and as soon as get the response, then fetch datafile again immdiately.
| Enabled: false | ||
| | ||
| Naming/AccessorMethodName: | ||
| Enabled: false |
There was a problem hiding this comment.
rather than disabling this cop for entire project, use ruby standard practice and rename get_config to only config.
See the good example here https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Naming/AccessorMethodName
Otherwise, only exclude specific files under this cop and not the whole project
.rubocop_todo.yml Outdated
| # Configuration parameters: CountKeywordArgs. | ||
| Metrics/ParameterLists: | ||
| Max: 6 | ||
| Max: 8 |
There was a problem hiding this comment.
exclude specific file under rubocop.yml instead of putting this in todo
| | ||
| # Get config for use by Optimizely. | ||
| # The config should be an instance of ProjectConfig. | ||
| def get_config(); end |
There was a problem hiding this comment.
we should raise a not implemented error here.
| return | ||
| end | ||
| | ||
| previous_revision = @config.revision if @config |
There was a problem hiding this comment.
The following code does not belong here. This should be part of polling config manager. In static we only initialize once with the provided datafile, that's it
There was a problem hiding this comment.
Polling config manager overrides this method!
lib/optimizely/helpers/constants.rb Outdated
| | ||
| # Default config update interval of 5 minutes | ||
| # Minimum config update interval of 1 second | ||
| # Time in seconds before which request for datafile times out |
There was a problem hiding this comment.
nit. Move these comments to a line above their explained constant
| | ||
| raise InvalidInputsError, 'Must provide at least one of sdk_key or url.' if sdk_key.nil? && url.nil? | ||
| | ||
| return (url_template % sdk_key) unless url |
There was a problem hiding this comment.
can this raise an exception if url_template doesn't have a placeholder or more? or the type is different? If yes, we should catch it
| require 'net/http' | ||
| | ||
| module Optimizely | ||
| class PollingConfigManager < StaticConfigManager |
There was a problem hiding this comment.
This should not be derived from the StaticConfigManager.
The StaticConfigManager should only be a holder for the proejct config and should implement the BaseConfigManager
The PollingConfigManager is another implementation of the BaseConfigManager.
Yes Mike, it was supposed to be merge in |
| Closing this as we've decided to split up into separate PRs. |
5 participants
Summary
This PR introduces config manager which polls for datafile after some interval and prepares project config based on it if it has updated.