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

Fix redirects #152

Merged
merged 1 commit into from
May 14, 2023
Merged

Fix redirects #152

merged 1 commit into from
May 14, 2023

Conversation

ShoshinNikita
Copy link
Contributor

Before this change redirects didn't work because method Service.extendMapper didn't copy the value of URLMapper.RedirectType to the extended result.

To fix this we return the original URLMapper instead of creating a new one (it can also help to avoid similar bugs in the future). We can reuse URLMapper because it is passed by value.

Before this change redirects didn't work because method `Service.extendMapper` didn't copy
the value of `URLMapper.RedirectType` to the extended result.

To fix this we return the original `URLMapper` instead of creating a new one (it can also help
to avoid similar bugs in the future). We can reuse `URLMapper` because it is passed
by value.
@ShoshinNikita
Copy link
Contributor Author

2022/11/07 13:47:25 INFO  activate http proxy server on 127.0.0.1:49700
    proxy_test.go:66: 
        	Error Trace:	proxy_test.go:66
        	Error:      	Received unexpected error:
        	            	Get "http://127.0.0.1:49700/api/something": dial tcp 127.0.0.1:49700: connect: connection refused
        	Test:       	TestHttp_Do
--- FAIL: TestHttp_Do (0.06s)

I reproduced this fail (~1/50 test runs). It looks like the 10ms delay (app/proxy/proxy_test.go:58) sometimes is not long enough. I can increase it to 20ms, for example. Another option is to just re-run CI.

@ShoshinNikita
Copy link
Contributor Author

2022/11/07 14:06:38.404 [INFO]  {logger/logger.go:134 logger.(*Middleware).Handler.func1.1} GET - /svc2/test - 127.0.0.1 - 127.0.0.1 - 404 (43) - 169.961115ms
main_test.go:81: 
      Error Trace:	main_test.go:81
      Error:      	Not equal: 
                    expected: 200
                    actual  : 404
      Test:       	Test_Main
main_test.go:84: 
      Error Trace:	main_test.go:84
      Error:      	"404 page not found\n" does not contain "echo echo 123"
      Test:       	Test_Main

app/main_test.go:28

"--static.rule=*,/svc2/(.*), https://echo.umputun.com/$1,https://feedmaster.umputun.com/ping",

It looks like https://echo.umputun.com/test doesn't work as expected anymore. I am not sure what caused this. Should I update expected status code and body?

@umputun
Copy link
Owner

umputun commented Nov 7, 2022

It looks like https://echo.umputun.com/test doesn't work as expected anymore

should be back

@ShoshinNikita
Copy link
Contributor Author

I think the decrease of coverage by 0.02% is not very critical. Especially, I believe it is caused by code removal.

Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

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

LGTM, thx a lot

@umputun umputun merged commit 2b92c11 into umputun:master May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants