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

Allow non root resources #454

Merged
merged 10 commits into from May 28, 2019
Merged

Allow non root resources #454

merged 10 commits into from May 28, 2019

Conversation

mangas
Copy link
Contributor

@mangas mangas commented May 24, 2019

#453

Changes

Added option allowNonRootResources
Added a function WrappedGrpcServer that encapsulates the logic of getting the gRPC endpoint from http.Request

Verification

Added Unit-Tests that ensure the same behavior for the canonical case
Tested with flag on by default

Copy link
Contributor

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

A few things, but generally this looks okay. I'd like approval from another maintainer as well as this is an API change.

go/grpcweb/helpers.go Outdated Show resolved Hide resolved
go/grpcweb/helpers_test.go Outdated Show resolved Hide resolved
go/grpcweb/wrapper.go Outdated Show resolved Hide resolved
@mangas
Copy link
Contributor Author

mangas commented May 24, 2019

Is it worth running one of the suites with the non default option to make sure that there is always something that tests the option?

@johanbrandhorst
Copy link
Contributor

Is it worth running one of the suites with the non default option to make sure that there is always something that tests the option?

I'd rather add a new test which exercises the option, but if it's complicated it's not necessary. it's a pretty straightforward change.

Copy link
Contributor

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

LGTM but I'd like a look from @jonny-improbable as well if possible before agreeing to merge this API change.

@mangas
Copy link
Contributor Author

mangas commented May 27, 2019

@johanbrandhorst @jonnyreeves I believe the failed tests are unrelated but let me know if there is anything I'm missing here

@johanbrandhorst
Copy link
Contributor

Restarted it

@mangas
Copy link
Contributor Author

mangas commented May 28, 2019

Will we be able to get some more 👀 on this PR? 🙏

@johanbrandhorst
Copy link
Contributor

🤞 lets hope tests pass

@johanbrandhorst
Copy link
Contributor

🔁

@mangas
Copy link
Contributor Author

mangas commented May 28, 2019

🔁

@johanbrandhorst
Copy link
Contributor

🙄 ♻️

@johanbrandhorst johanbrandhorst merged commit e63b987 into improbable-eng:master May 28, 2019
@johanbrandhorst
Copy link
Contributor

Thanks for your contribution!

@mangas
Copy link
Contributor Author

mangas commented May 28, 2019

Thank you for the help 👍

@mangas mangas deleted the allow-non-root-resources branch May 28, 2019 19:29
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