feat: add refetch option and update dependencies#16
Conversation
def9156 to
44e564f
Compare
llxff
left a comment
There was a problem hiding this comment.
You also need to bump version in the version constant to publish a new version.
All changes look great, thanks!
| with_decryption: true | ||
| ).parameter.value | ||
| def query_ssm_parameter(name, refetch) | ||
| if refetch || !@items_cache.key?(name) |
There was a problem hiding this comment.
Checking for key is a nice improvement! Thanks!
Gemfile
Outdated
| group :development do | ||
| gem 'bundler', '~> 2.6.9' | ||
| gem 'rake', '~> 13.0' | ||
| gem 'standard', '>= 1.50.0' |
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes the codebase by updating dependencies, migrating from RuboCop to StandardRB, and adding a new refetch option to force cache bypassing in configuration adapters. The changes include Ruby version updates, Docker configuration improvements, and comprehensive code style standardization.
- Updates Ruby version from 2.7.3 to 3.2.8 and modernizes Docker/CI configurations
- Replaces RuboCop with StandardRB for code formatting and applies style fixes throughout
- Adds
refetchparameter to configuration adapters allowing cache bypass for fresh data retrieval
Reviewed Changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/blinkist/config.rb | Adds refetch parameter to get methods and removes unnecessary return statement |
| lib/blinkist/config/adapters/aws_ssm_adapter.rb | Implements refetch functionality and modernizes string interpolation |
| lib/blinkist/config/adapters/diplomat_adapter.rb | Adds refetch support and consolidates cache logic |
| spec/blinkist/config_spec.rb | Updates test expectations to include refetch: false parameter |
| spec/blinkist/adapters/aws_ssm_adapter_spec.rb | Adds comprehensive tests for refetch functionality |
| spec/blinkist/adapters/diplomat_adapter_spec.rb | Adds tests for refetch behavior |
| blinkist-config.gemspec | Updates Ruby version requirement and dependency management |
| docker-compose.yml | Removes deprecated version specification |
| Dockerfile | Updates Ruby version and improves container setup |
| README.md | Documents new refetch feature and updates Docker commands |
Comments suppressed due to low confidence (2)
spec/blinkist/adapters/aws_ssm_adapter_spec.rb:48
- [nitpick] The test doesn't verify that the first call actually cached the value. Consider adding an assertion to verify the first_value matches the expected cached value.
first_value = adapter.get(key, default, scope: scope)
spec/blinkist/adapters/diplomat_adapter_spec.rb:51
- [nitpick] Similar to the AWS SSM adapter test, this test doesn't verify that the first call actually cached the value. Consider adding an assertion to verify the first_value matches the expected cached value.
first_value = adapter.get(key, default, scope: scope)
| if refetch || !@items_cache.key?(name) | ||
| @items_cache[name] = @client.get_parameter( | ||
| name: name, | ||
| with_decryption: true | ||
| ).parameter.value | ||
| end | ||
| @items_cache[name] | ||
| end | ||
|
|
There was a problem hiding this comment.
[nitpick] The refetch logic could be extracted to a separate method for better readability and to reduce the conditional complexity in query_ssm_parameter.
| if refetch || !@items_cache.key?(name) | |
| @items_cache[name] = @client.get_parameter( | |
| name: name, | |
| with_decryption: true | |
| ).parameter.value | |
| end | |
| @items_cache[name] | |
| end | |
| fetch_ssm_parameter(name) if refetch || !@items_cache.key?(name) | |
| @items_cache[name] | |
| end | |
| def fetch_ssm_parameter(name) | |
| @items_cache[name] = @client.get_parameter( | |
| name: name, | |
| with_decryption: true | |
| ).parameter.value | |
| end |
| first_value = adapter.get(key, default, scope: scope) | ||
|
|
||
| # Second call should get a new value due to refetch: true | ||
| allow(ssm_client).to receive_message_chain(:get_parameter, :parameter, :value).and_return(new_value) |
There was a problem hiding this comment.
Using receive_message_chain is generally discouraged in RSpec as it can make tests brittle. Consider using allow(ssm_client).to receive(:get_parameter).and_return(double(parameter: double(value: new_value))) instead.
| allow(ssm_client).to receive_message_chain(:get_parameter, :parameter, :value).and_return(new_value) | |
| allow(ssm_client).to receive(:get_parameter).and_return(double(parameter: double(value: new_value))) |
There was a problem hiding this comment.
Naja. At least I would not use double, because it doesn’t guarantee that the actual implementation really returns an object with the given contract. Instead I would use instance_double.
To follow the readme I had to update quite some config for docker to work. I also took the liberty to update rubocop and so on :-)