Skip to content

Upgrade to Ruby 2.3#1781

Merged
ysbaddaden merged 7 commits intomainfrom
tech-debt/upgrade-to-ruby-2.3
Nov 25, 2022
Merged

Upgrade to Ruby 2.3#1781
ysbaddaden merged 7 commits intomainfrom
tech-debt/upgrade-to-ruby-2.3

Conversation

@ysbaddaden
Copy link
Copy Markdown
Contributor

@ysbaddaden ysbaddaden commented Nov 7, 2022

The upgrade went almost smoothly, if not for a regression in the Policy System. Once we identify and fix that bug, then we can upgrade to Ruby 2.3 with high confidence.

  • Fix 1 spec that hangs forever (disabled for now):

    • spec/models/policy_spec.rb:271 # Policy Institution Read allows checking when there's a loop
  • Fix 15 failing policy specs (probably the same issue):

    • rspec ./spec/models/test_result_query_spec.rb:43 # TestResultQuery policies delegates all tests from a user
    • rspec ./spec/models/test_result_query_spec.rb:123 # TestResultQuery policies have access with policy by site with children
    • rspec ./spec/models/test_result_query_spec.rb:99 # TestResultQuery policies have access with policy by institution
    • rspec ./spec/models/policy_spec.rb:89 # Policy Authorize should return an instance when queried over an instance
    • rspec ./spec/models/policy_spec.rb:77 # Policy Authorize should return a scope when queried over a class
    • rspec ./spec/models/policy_spec.rb:790 # Policy Test Result Query allows to query test result by site
    • rspec ./spec/models/policy_spec.rb:815 # Policy Test Result Query returns a scope with tests authorised by device
    • rspec ./spec/models/policy_spec.rb:785 # Policy Test Result Query allows to query test result by institution
    • rspec ./spec/models/policy_spec.rb:795 # Policy Test Result Query allows to query test result by device
    • rspec ./spec/models/policy_spec.rb:800 # Policy Test Result Query returns a scope with tests authorised by institution
    • rspec ./spec/models/policy_spec.rb:805 # Policy Test Result Query returns a scope with tests authorised by site
    • rspec ./spec/models/policy_spec.rb:826 # Policy Test Result Query returns a scope with all tests
    • rspec ./spec/models/policy_spec.rb:820 # Policy Test Result Query returns a scope with tests authorised by multiple criteria
    • rspec ./spec/models/computed_policy_spec.rb:165 # ComputedPolicy from regular user should compact identical rules in policies
    • rspec ./spec/controllers/api/sites_controller_spec.rb:72 # Api::SitesController Sites hierarchical READ_SITE should propagate to the childs
  • Fix new failing spec:

    • rspec ./spec/models/computed_policy_spec.rb:340 # ComputedPolicy recursively should support a loop of policies

@ysbaddaden
Copy link
Copy Markdown
Contributor Author

The call to #grant in specs only creates a Policy object and doesn't create the required entries to ComputedPolicy, hence not granting the actual permissions (those that are checked). The bug is likely in PolicyComputer#update_user, which states that it has nothing to create when it should.

@ysbaddaden
Copy link
Copy Markdown
Contributor Author

Further cornered to PolicyComputer#compact_policies and... fixed.

Ruby 2.3 doesn't like that we iterate an Array from which we're currently deleting. Using a distinct Array fixed the bug.

@ysbaddaden
Copy link
Copy Markdown
Contributor Author

Down to 1 failing spec, which tells me I fixed the issue of not computing policies but failed the compaction (de-duplicating policies).

@ysbaddaden ysbaddaden marked this pull request as ready for review November 8, 2022 09:49
@ysbaddaden
Copy link
Copy Markdown
Contributor Author

ysbaddaden commented Nov 8, 2022

Urgh, actually I only updated the development environment to Ruby 2.3, the production is still running Ruby 2.2, but hopefully we already have the instedd/nginx-rails:2.3 image ready:
https://hub.docker.com/layers/instedd/nginx-rails/2.3/images/sha256-e38b708ef47c2656633daac44f6e74b719e270c4095fdf10e7a23f9c592ebb31?context=explore

@diegoliberman diegoliberman linked an issue Nov 14, 2022 that may be closed by this pull request
@ysbaddaden ysbaddaden force-pushed the tech-debt/upgrade-to-ruby-2.3 branch from 0529881 to efac87c Compare November 14, 2022 13:41
@ysbaddaden ysbaddaden changed the title Tech debt/upgrade to ruby 2.3 Upgrade to Ruby 2.3 Nov 14, 2022
@ysbaddaden ysbaddaden self-assigned this Nov 14, 2022
@ysbaddaden ysbaddaden mentioned this pull request Nov 15, 2022
19 tasks
@ysbaddaden ysbaddaden merged commit 532c3ec into main Nov 25, 2022
@ysbaddaden ysbaddaden deleted the tech-debt/upgrade-to-ruby-2.3 branch November 25, 2022 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrade to ruby 2.3

2 participants