Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge all routing policies into one #1465

Merged
merged 1 commit into from Dec 4, 2019

Conversation

guicassolato
Copy link
Contributor

@guicassolato guicassolato commented Nov 29, 2019

This is one of the options we have to fix THREESCALE-4016 It corresponds to the first solution proposed in the dev notes.

@guicassolato guicassolato self-assigned this Nov 29, 2019
@Martouta Martouta requested a review from a team December 2, 2019 09:00
@guicassolato guicassolato marked this pull request as ready for review December 3, 2019 13:35
@guicassolato guicassolato force-pushed the fix/multiple-routing-policies branch 2 times, most recently from c3c194d to 0d82898 Compare December 3, 2019 14:29
@codecov
Copy link

codecov bot commented Dec 3, 2019

Codecov Report

Merging #1465 into master will decrease coverage by 27.29%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1465      +/-   ##
==========================================
- Coverage   90.47%   63.18%   -27.3%     
==========================================
  Files        2252     1509     -743     
  Lines       70471    40928   -29543     
==========================================
- Hits        63760    25861   -37899     
- Misses       6711    15067    +8356
Impacted Files Coverage Δ
app/lib/backend_api_logic/routing_policy.rb 47.22% <14.28%> (-52.78%) ⬇️
app/indices/account_index.rb 5.55% <0%> (-88.89%) ⬇️
app/indices/topic_index.rb 14.28% <0%> (-85.72%) ⬇️
...pp/controllers/buyers/impersonations_controller.rb 15.38% <0%> (-84.62%) ⬇️
app/lib/csv/applications_exporter.rb 16% <0%> (-84%) ⬇️
config/abilities/buyer_any.rb 5% <0%> (-90%) ⬇️
test/test_helpers/proxy.rb 20% <0%> (-80%) ⬇️
app/indices/proxy_rules_index.rb 20% <0%> (-80%) ⬇️
app/helpers/vertical_nav_helper.rb 12.22% <0%> (-81.67%) ⬇️
app/messengers/plans_messenger.rb 22.22% <0%> (-77.78%) ⬇️
... and 1434 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e588416...3381ed0. Read the comment docs.


routing_policy = Builder.new(service).to_h

return other_policies if routing_policy['configuration']['rules'].concat(other_routing_rules).empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

rules can be nil in some policies. But agree that for routing it must be an Array

[{"configuration":{},"name":"apicast","version":"builtin","enabled":true},{"configuration":{"rules":[]},"name":"routing","version":"builtin","enabled":true}]

@guicassolato guicassolato merged commit 4977bc0 into master Dec 4, 2019
@guicassolato guicassolato deleted the fix/multiple-routing-policies branch December 4, 2019 09:54
guicassolato added a commit that referenced this pull request Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants