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.

Sign up for GitHub

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

Jump to bottom

Add configuration support for nyhoor websocket read limit #1023

Merged
merged 2 commits into from
Jul 10, 2021
Merged

Add configuration support for nyhoor websocket read limit #1023

merged 2 commits into from
Jul 10, 2021

Conversation

bryanmcgrane
Copy link
Contributor

Adding support to configure the underlying nyhoor websocket read limit.

Changes

The changes are minimal

  1. Add new configuration property WithWebsocketsMessageReadLimit
  2. Apply this read limit to the websocket if it is configured, otherwise do nothing. In this case the default limit is used.

Verification

The codes successfully builds and works in my local environment.

@improbable-prow-robot improbable-prow-robot added the size/S Denotes a PR that changes 15-39 lines, ignoring generated files. label Jul 8, 2021
@bryanmcgrane bryanmcgrane mentioned this pull request Jul 8, 2021
Open
@bryanmcgrane
Copy link
Contributor Author

FYI - the build failed due to changes outside of my PR. It looks like the go.mod file has some linting issues.

johanbrandhorst
johanbrandhorst suggested changes Jul 9, 2021
View reviewed changes
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.

Hi, thanks for this change! I think it's missing an additional flag for the grpcwebproxy standalone proxy that could configure this setting for the standalone proxy. What do you think?

@bryanmcgrane
Copy link
Contributor Author

Good thinking. That change has been made as well!

@johanbrandhorst
Copy link
Contributor

Looks like CI is failing because of some module inconsistencies. Could you run go mod tidy and commit the changes?

johanbrandhorst
johanbrandhorst approved these changes Jul 10, 2021
View reviewed changes
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.

Scratch that, I don't see why this change should cause any import changes.

@johanbrandhorst johanbrandhorst merged commit 53e1aaa into improbable-eng:master Jul 10, 2021
@johanbrandhorst
Copy link
Contributor

Thanks for your contribution!

@bryanmcgrane
Copy link
Contributor Author

bryanmcgrane commented Jul 12, 2021 •
edited
Loading

You're welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johanbrandhorst johanbrandhorst johanbrandhorst approved these changes

Assignees
No one assigned
Labels
size/S Denotes a PR that changes 15-39 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bryanmcgrane @johanbrandhorst @improbable-prow-robot