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

feat: make request timeout configurable #69

Merged
merged 4 commits into from
Sep 6, 2022

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Sep 2, 2022

The 1s timeout is pretty long but currently my ftw tests don't pass with it - I need to debug whether this is an Envoy configuration issue, wasm performance issue, whether it affects real traffic, etc, and hopefully it won't be slow. But in the meantime, being able to run the test suite by configuring a higher timeout would be really great.

It looks like some files weren't gofmt'd and my IDE automatically does it, I hope that's OK

Copy link
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

I like the general idea.

One thing that is bothering me now is that the Run is taking way too many parameters now, and probably changing it to a RunOptions functional style will be more readable and easy to update. What do you think?

If you want to push a new PR with that, I can merge this one in the meantime.

Copy link
Contributor Author

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks @fzipi - added a Config struct

runner/run_test.go Show resolved Hide resolved
runner/run_test.go Show resolved Hide resolved
@fzipi
Copy link
Member

fzipi commented Sep 5, 2022

Looks good! The only thing that I see is that there is a lot of Configs going around now :)

If you feel good about it, let's merge. I need to review afterward how is this going to look when used programmatically.

@fzipi
Copy link
Member

fzipi commented Sep 5, 2022

Just to be in sync. From this doc I was thinking of option 3, and you implemented option 2.

@anuraaga
Copy link
Contributor Author

anuraaga commented Sep 6, 2022

Thanks @fzipi - yeah I think this is good to merge. Currently have to build from my fork so will be glad to switch back 😃

I tend to have a preference for option 2 mostly since it gives easier discoverability to the options just IMO so went with it. Reworking to 3 would be fine though. Just some notes from wazero in case interesting.

https://github.com/tetratelabs/wazero/blob/main/RATIONALE.md#why-arent-configuration-assigned-with-option-types

@fzipi
Copy link
Member

fzipi commented Sep 6, 2022

No worries, for now it makes sense, and simplifies the work. Let's merge and see it in action.

@fzipi fzipi merged commit 67881bf into coreruleset:main Sep 6, 2022
@anuraaga anuraaga mentioned this pull request Sep 6, 2022
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.

2 participants