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

Handler middlewares #1019

Merged
merged 6 commits into from
Sep 24, 2024
Merged

Handler middlewares #1019

merged 6 commits into from
Sep 24, 2024

Conversation

marefr
Copy link
Member

@marefr marefr commented Jul 5, 2024 •
edited
Loading

Moves https://github.com/grafana/grafana/blob/22df2d9b06b0b3651dcd1d2c075ffd48071ab556/pkg/plugins/ifaces.go#L137-L152 into the SDK.

At first I wanted to create a new handlermiddleware or similar package, but it would depend on the backend package and soon (follow up) the backend package is going to make use of handler middlewares and if so create a cyclic dependency 😢

Implemented/used in grafana/grafana#93445

One breaking change renaming a constant (why Detect incompatible changes fails), but is related to experimental code and shouldn't affect plugins:

## incompatible changes
EndpointConvertObject: removed
## compatible changes
EndpointConvertObjects: added

@marefr marefr self-assigned this Jul 5, 2024
@marefr marefr force-pushed the handler_middleware branch 2 times, most recently from 70d3460 to 78d7342 Compare July 9, 2024 08:49
@marefr marefr mentioned this pull request Jul 10, 2024
Merged
@ivanahuckova ivanahuckova mentioned this pull request Sep 12, 2024
Open
@marefr marefr mentioned this pull request Sep 18, 2024
Merged
3 tasks
@marefr marefr marked this pull request as ready for review September 18, 2024 15:13
@marefr marefr requested a review from a team as a code owner September 18, 2024 15:13
@marefr marefr requested review from wbrowne, andresmgot and oshirohugo and removed request for a team September 18, 2024 15:13
marefr
marefr commented Sep 18, 2024
View reviewed changes
backend/handler_middleware.go Outdated Show resolved Hide resolved
andresmgot
andresmgot approved these changes Sep 19, 2024
View reviewed changes
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

LGTM, just some questions / nits

backend/handler_middleware.go Outdated Show resolved Hide resolved
backend/handler_middleware.go Outdated Show resolved Hide resolved
backend/handler_middleware.go Show resolved Hide resolved
wbrowne
wbrowne reviewed Sep 20, 2024
View reviewed changes
Copy link
Member

@wbrowne wbrowne left a comment

Choose a reason for hiding this comment

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

Looks good - just one thing that is missing AFAICT

backend/handler_middleware.go
ctx = initErrorSource(ctx)
ctx = WithEndpoint(ctx, endpoint)
ctx = WithPluginContext(ctx, pluginCtx)
ctx = WithGrafanaConfig(ctx, pluginCtx.GrafanaConfig)
Copy link
Member

Choose a reason for hiding this comment

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

It's only now when looking at all this when I realize all we really need to pass through for a lot of these is pluginCtx since these are all exported fields that could be read as needed 🤦

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah 😄 What do you suggest, remove WithGrafanaConfig and use PluginConfigFromContext in GrafanaConfigFromContext? Or remove GrafanaConfigFromContext altogether?

Copy link
Member

Choose a reason for hiding this comment

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

If we remove WithGrafanaConfig it'll break Grafana core - and all the core plugins would need to be updated.

Removing GrafanaConfigFromContext will smash a bunch of plugins in the wild.

So I guess for now we do nothing, except cry 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah 😄 On the positive side, the current way allow tests to use backend. WithGrafanaConfig rather than creating a full context.

backend/handler_middleware.go Outdated Show resolved Hide resolved
backend/stream.go Show resolved Hide resolved
backend/stream.go Outdated Show resolved Hide resolved
marefr and others added 2 commits September 23, 2024 14:48
@marefr @wbrowne
Update backend/stream.go
4962c23 
Co-authored-by: Will Browne <wbrowne@users.noreply.github.com>
@marefr
updates after review
f2a6b93
andresmgot
andresmgot approved these changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

LGTM

wbrowne
wbrowne approved these changes Sep 24, 2024
View reviewed changes
Copy link
Member

@wbrowne wbrowne left a comment

Choose a reason for hiding this comment

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

🚀

@marefr marefr merged commit d44c599 into main Sep 24, 2024
2 of 3 checks passed
@marefr marefr deleted the handler_middleware branch September 24, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wbrowne wbrowne wbrowne approved these changes

@andresmgot andresmgot andresmgot approved these changes

@oshirohugo oshirohugo Awaiting requested review from oshirohugo oshirohugo is a code owner automatically assigned from grafana/plugins-platform-backend

Assignees

@marefr marefr

Labels
None yet
Projects
Plugins Platform / Grafana Community
Status: 🚀 Shipped
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marefr @wbrowne @andresmgot