-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Fix: plugin loader cache not matching on decoupled plugin paths #108529
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
devenv/local_cdn/default.conf
Outdated
@@ -19,6 +19,6 @@ server { | |||
add_header 'Access-Control-Allow-Headers' '*' always; | |||
add_header 'Access-Control-Allow-Credentials' 'true' always; | |||
|
|||
rewrite ^/grafana-oss/11.4.0-pre/public(.*)$ $1 last; | |||
rewrite ^/grafana/12.1.0-pre/public(.*)$ $1 last; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it makes sense to keep this as the non OSS version from now on 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing something or this functionality doesn't fully work in OSS version now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually maybe it doesn't make sense to change. Core plugins in OSS won't make use of this feature - but this change means OSS assets won't get loaded, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't get anything to load whilst testing this without making this change but maybe I made a mistake somewhere down the road to getting this all running locally for this bug fix. I'll revert the -oss
part. The version change is legit.
What is this feature?
This PR fixes an issue due to Decoupled core plugins now loading from different asset paths to regular plugins which prevented the loader cache (which stores information related to which version to append to the cache buster query param and loadingStrategy) from having any effect.
Why do we need this feature?
So decoupled plugins continue to load using the version as a cache buster and the correct loading strategy
Who is this feature for?
Everyone.
Which issue(s) does this PR fix?:
Fixes #107132
Special notes for your reviewer:
Please check that: