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

Avoid unwanted path stripping #3765

Open
wants to merge 7 commits into
base: 2.2.x
Choose a base branch
from
Open

Conversation

OskarD
Copy link

@OskarD OskarD commented Apr 3, 2020

@pivotal-issuemaster
Copy link

@OskarD Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@OskarD Thank you for signing the Contributor License Agreement!

@codecov
Copy link

codecov bot commented Apr 3, 2020

Codecov Report

Merging #3765 into 2.2.x will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##              2.2.x    #3765      +/-   ##
============================================
- Coverage     65.86%   65.83%   -0.03%     
+ Complexity     1594     1593       -1     
============================================
  Files           204      204              
  Lines          7455     7455              
  Branches        876      876              
============================================
- Hits           4910     4908       -2     
- Misses         2226     2227       +1     
- Partials        319      320       +1     
Impacted Files Coverage Δ Complexity Δ
...cloud/netflix/zuul/filters/SimpleRouteLocator.java 89.52% <100.00%> (ø) 34.00 <0.00> (ø)
...ework/cloud/netflix/archaius/ArchaiusEndpoint.java 81.81% <0.00%> (-9.10%) 7.00% <0.00%> (-1.00%)

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 74f9fd4...55b42ac. Read the comment docs.

It will still fail, but I think it's a different bug
@OskarD
Copy link
Author

OskarD commented Apr 3, 2020

I'm guessing the test will fail with something like this:

Expecting:
<Route{id='foo-bar', fullPath='/foo-bar/1', path='/1', location='foo', prefix='/foo-bar', retryable=false, sensitiveHeaders=[], customSensitiveHeaders=false, prefixStripped=true}>
to be equal to:
<Route{id='foobar', fullPath='/foo-bar/1', path='/1', location='foo', prefix='/foo-bar', retryable=false, sensitiveHeaders=[], customSensitiveHeaders=false, prefixStripped=true}>
but was not.

But I don't understand why? Why does the ID suddenly have the hyphen?

@spencergibb
Copy link
Member

Looking good. Is there an edge case where just /foo (ie context) is routed thru zuul? Maybe test that and see what happens.

@OskarD
Copy link
Author

OskarD commented Apr 5, 2020

Looking good. Is there an edge case where just /foo (ie context) is routed thru zuul? Maybe test that and see what happens.

It seems to be working, although I don't understand why SimpleRouteLocator::getRoute is mocked

I looked closer at the ID thing, and I get it now... although it is a little confusing

@OskarD OskarD changed the title Avoid unwanted stripping of path Avoid unwanted path stripping Apr 6, 2020
@OskarD OskarD requested a review from spencergibb April 8, 2020 09:28
@OskarD
Copy link
Author

OskarD commented Apr 8, 2020

Why is codecov/project failing? When I click details I get "Unable to find report content in the storage archive."

@spencergibb
Copy link
Member

Don't worry about codecov

@OskarD
Copy link
Author

OskarD commented Apr 14, 2020

@spencergibb will this be merged?

@OskarD
Copy link
Author

OskarD commented May 12, 2020

@spencergibb ping

@spencergibb
Copy link
Member

I haven't had a chance to review it since the updates.

@spencergibb
Copy link
Member

The new tests still don't fail if I remove the change to SimpleRouteLocator. Without this I can't tell if there is a regression or now.

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.

None yet

4 participants