Cast boolean values #45

i7an · 2024-11-22T09:38:16Z

Boolean methods return raw values. AWS Parameters Store does not support boolean type.

Global.foo.enabled? # => "false"

if Global.foo.enabled? # "false" is truthy
  # ...
end

In this PR I attempted to address this problem:

Global.foo.enabled # => "false"
Global.foo.enabled.class # => String
Global.foo.enabled? # => false
Global.foo.enabled?.class # => FalseClass

`rspec/core` is loaded by default.

i7an · 2024-11-22T09:40:00Z

Rakefile

-require 'rubygems'
-require 'bundler'
-
-Bundler.require


https://bundler.io/guides/bundler_setup.html

For such a small Gemfile, we’d advise you to skip Bundler.require and just require the gems by hand

i7an · 2024-11-22T09:40:22Z

Rakefile

@@ -1,12 +1,7 @@
 # frozen_string_literal: true

-require 'rubygems'
-require 'bundler'


Did not find why these were needed.

i7an · 2024-11-22T09:41:55Z

lib/global/backend/aws_parameter_store.rb

@@ -95,7 +95,7 @@ def load_parameters_from_ssm(next_token = nil)
      def build_configuration_from_parameters(parameters)
        configuration = {}
        parameters.each do |parameter|
-          parameter_parts = parameter.name[@prefix.length..-1].split(PATH_SEPARATOR).map(&:to_sym)
+          parameter_parts = parameter.name[@prefix.length..].split(PATH_SEPARATOR).map(&:to_sym)


rubocop autofix. applies to other as well.

i7an · 2024-11-22T09:43:13Z

lib/global/configuration.rb

+      'off', :off,
+      'OFF', :OFF
+    ].to_set.freeze
+    private_constant :FALSE_VALUES


FALSE_VALUES is implementation details and should not be exposed.

i7an · 2024-11-22T09:43:39Z

lib/global/configuration.rb

+      normalized_method = normalize_key_by_method(method)
+      if key?(normalized_method)
+        value = get_configuration_value(normalized_method)
+        boolean_method?(method) ? cast_boolean(value) : value


this is what this PR is all about.

i7an · 2024-11-22T09:44:35Z

spec/global/backend/aws_parameter_store_spec.rb

 require 'aws-sdk-ssm'
 require 'global/backend/aws_parameter_store'

 RSpec.describe Global::Backend::AwsParameterStore do
+  subject(:parameter_store) do
+    described_class.new(prefix: '/testapp/', client: client)
+  end


fixed rubocop abuses. applies to other spec files.

i7an · 2024-11-22T09:45:42Z

spec/spec_helper.rb

@@ -1,9 +1,5 @@
 # frozen_string_literal: true

-$LOAD_PATH.unshift(File.join(File.dirname(__FILE__), '..', 'lib'))
-$LOAD_PATH.unshift(File.dirname(__FILE__))


spec and lib directories are in the load path by default

i7an · 2024-11-22T09:45:53Z

spec/spec_helper.rb

@@ -1,9 +1,5 @@
 # frozen_string_literal: true

-$LOAD_PATH.unshift(File.join(File.dirname(__FILE__), '..', 'lib'))
-$LOAD_PATH.unshift(File.dirname(__FILE__))
-require 'rspec'


rspec/core is loaded by default

i7an · 2024-11-22T09:46:43Z

spec/spec_helper.rb

-$LOAD_PATH.unshift(File.join(File.dirname(__FILE__), '..', 'lib'))
-$LOAD_PATH.unshift(File.dirname(__FILE__))
-require 'rspec'
-require 'global'


simplecov should be required before any code is loaded
https://github.com/simplecov-ruby/simplecov?tab=readme-ov-file#troubleshooting

leonid-shevtsov

I suppose the ruby version range is acceptable, seeing as 3.0 is already EOL

i7an · 2024-11-22T10:53:11Z

.github/workflows/tests.yml

+          - 3.3
+          - 3.2
+          - 3.1
+          - 3.0


le0pard

need some fixes

le0pard · 2024-11-26T14:25:36Z

global.gemspec


 Gem::Specification.new do |s|
  s.name = 'global'
  s.version = Global::VERSION
+  s.required_ruby_version = '>= 3.0.0'


This may require by semver release major version

le0pard · 2024-11-29T09:01:45Z

global.gemspec

  s.summary = 'Simple way to load your configs from yaml/aws/gcp'

+  s.metadata['rubygems_mfa_required'] = 'true'


Not sure, we activated this and how ci will do release

@le0pard could you please share the script that does the release? I want to check that.

how ci will do release

I assumed there is CI that pushed a new version automatically. Ok.
Could you please check if MFA is on or off? If not maybe we should turn it on?

le0pard · 2024-11-29T10:10:00Z

lib/global/configuration.rb

@@ -61,6 +77,11 @@ def boolean_method?(method)
      '?' == method.to_s[-1]
    end

+    # @see ActiveModel::Type::Boolean#cast_value
+    def cast_boolean(value)
+      !FALSE_VALUES.include?(value)


so empty string will be true? should it be nil...

le0pard · 2024-11-29T10:10:47Z

lib/global/version.rb

@@ -2,6 +2,6 @@

 module Global

-  VERSION = '2.1.0'
+  VERSION = '2.2.0'


nope, new major version - removed ruby 2 support

le0pard · 2024-11-29T10:11:17Z

spec/global/backend/aws_parameter_store_spec.rb


-  it 'reads parameters from the parameter store' do
+  it 'reads parameters from the parameter store' do # rubocop:disable RSpec/ExampleLength, RSpec/MultipleExpectations


RSpec/MultipleExpectations - you can fix this

I tried several options. next_token expectations is the tricky part. If you want to keep them, you have to adjust rubocop configuration. To reduce the number of lines again you have to deal with next_token expectations, move stubs to elsewhere which does not improve code readability.
At the end I think disabling those two rules for one it is fine. If you disagree please suggest how to deal with these offences.

le0pard · 2024-11-29T10:11:55Z

spec/global/configuration_spec.rb

+    {
+      'key' => 'value',
+      'boolean_key' => true,
+      'string_boolean_key' => '1',


test with empty string and false value

le0pard · 2024-11-29T10:12:31Z

spec/global_merge_backends_spec.rb

+  subject(:global) { described_class }
+
+  describe 'merging backends' do
+    # rubocop:disable RSpec/VerifiedDoubles


this can be fixed and written correctly

Global takes an object that quacks like a configuration. This spec tests merging, thus abstract doubles are legit.

i7an · 2024-12-04T10:15:50Z

spec/global/backend/gcp_secret_manager_spec.rb


-    @not_match_item = double
-    allow(@not_match_item).to receive(:name).and_return('different_key')
+    secret_data = double(data: 'secret value')


🤷‍♂️

Google::Cloud::SecretManager::V1::SecretPayload.new.data # => "" instance_double(Google::Cloud::SecretManager::V1::SecretPayload, data: 'secret value') # => RSpec::Mocks::MockExpectationError: class does not implement the instance method: data

i7an added 8 commits November 19, 2024 19:12

updated rubocop and fixed offenses

fda173b

updated gitignore

13f9d8e

test ruby 3.x only

feb75b6

cast boolean values

f0c06be

remove $LOAD_PATH calls

65f593a

require spec_helper by default

dcd2152

init simplecov before everything else

9a634d1

removed explicit rspec require

9022932

`rspec/core` is loaded by default.

i7an commented Nov 22, 2024

View reviewed changes

leonid-shevtsov approved these changes Nov 22, 2024

View reviewed changes

i7an commented Nov 22, 2024

View reviewed changes

.github/workflows/tests.yml

- 3.3

- 3.2

- 3.1

- 3.0

Copy link

Contributor Author

i7an Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leonid-shevtsov reacted with thumbs up emoji

le0pard reviewed Nov 29, 2024

View reviewed changes

i7an added 3 commits November 29, 2024 12:02

handle casting empty string and nil to boolean

16706e2

replaced some doubles with instance doubles

c6c1a11

v3.0.0

66b555b

i7an force-pushed the cast_booleans branch from 9affed6 to 66b555b Compare December 4, 2024 10:13

i7an commented Dec 4, 2024

View reviewed changes

i7an requested a review from le0pard December 4, 2024 10:16

le0pard approved these changes Dec 4, 2024

View reviewed changes

le0pard merged commit 2d442e3 into railsware:master Dec 4, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cast boolean values #45

Cast boolean values #45

i7an commented Nov 22, 2024 •

edited

Loading

i7an Nov 22, 2024

i7an Nov 22, 2024

i7an Nov 22, 2024

i7an Nov 22, 2024

i7an Nov 22, 2024

i7an Nov 22, 2024

i7an Nov 22, 2024

i7an Nov 22, 2024

i7an Nov 22, 2024

leonid-shevtsov left a comment

i7an Nov 22, 2024

le0pard left a comment

le0pard Nov 26, 2024

le0pard Nov 29, 2024

i7an Nov 29, 2024

le0pard Nov 29, 2024

i7an Dec 3, 2024 •

edited

Loading

le0pard Nov 29, 2024

le0pard Nov 29, 2024

le0pard Nov 29, 2024

i7an Dec 3, 2024

le0pard Nov 29, 2024

le0pard Nov 29, 2024

i7an Nov 29, 2024 •

edited

Loading

i7an Dec 4, 2024

		s.summary = 'Simple way to load your configs from yaml/aws/gcp'

		s.metadata['rubygems_mfa_required'] = 'true'


		it 'reads parameters from the parameter store' do
		it 'reads parameters from the parameter store' do # rubocop:disable RSpec/ExampleLength, RSpec/MultipleExpectations

Cast boolean values #45

Cast boolean values #45

Conversation

i7an commented Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonid-shevtsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

le0pard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i7an Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i7an Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i7an commented Nov 22, 2024 •

edited

Loading

i7an Dec 3, 2024 •

edited

Loading

i7an Nov 29, 2024 •

edited

Loading