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 server version middleware to serve-testing #2006

Merged
merged 1 commit into from
Aug 5, 2024
Merged

Add server version middleware to serve-testing #2006

merged 1 commit into from
Aug 5, 2024

Conversation

josephschorr
Copy link
Member

Fixes #2003

@josephschorr josephschorr requested a review from a team as a code owner July 30, 2024 21:30
@github-actions github-actions bot added the area/CLI Affects the command line label Jul 30, 2024
vroldanbet
vroldanbet previously requested changes Jul 31, 2024
View reviewed changes
Copy link
Contributor

@vroldanbet vroldanbet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can you reuse the defaults here instead? That way we won't have drifting in the future. It also uses the middleware framework, which is more robust when it comes to dependencies between middlewares.

@josephschorr
Copy link
Member Author

Can you reuse the defaults here instead? That way we won't have drifting in the future. It also uses the middleware framework, which is more robust when it comes to dependencies between middlewares.

serve-testing doesn't use most of those defaults and we've have to replace a few of them. I suppose we could inject/override some, but that has its own risks

@josephschorr
Copy link
Member Author

Updated to use the middleware system as we discussed

@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Aug 1, 2024
ecordell
ecordell reviewed Aug 5, 2024
View reviewed changes
pkg/cmd/server/defaults.go
enableRequestLog bool
enableResponseLog bool
disableGRPCHistogram bool
Logger zerolog.Logger `debugmap:"hidden"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are all of these exported now? I don't see that being used anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Figured since the With* methods are now exported, its a public config

@josephschorr josephschorr added this pull request to the merge queue Aug 5, 2024
Merged via the queue into authzed:main with commit 54cace8 Aug 5, 2024
22 checks passed
@josephschorr josephschorr deleted the version-middleware-serve-test branch August 5, 2024 18:46
@github-actions github-actions bot locked and limited conversation to collaborators Aug 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ecordell ecordell ecordell approved these changes

@vroldanbet vroldanbet vroldanbet left review comments

Assignees
No one assigned
Labels
area/CLI Affects the command line area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SpiceDB version middleware not enabled in serve-testing mode
3 participants
@josephschorr @ecordell @vroldanbet