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

Plugins: Don't auto prepend app sub url to plugin asset paths #81658

Merged
merged 20 commits into from
Feb 8, 2024

Conversation

wbrowne
Copy link
Member

@wbrowne wbrowne commented Jan 31, 2024

What is this feature?

This moves the prefixing of appSubUrl for plugin assets to the frontend so it can be dynamically appended depending on the request.

This is required because under certain conditions the image renderer changes the config.appSubUrl before making the request to Grafana backend. This leads to frontend asset paths for Grafana and external plugins being mismatched as the base and module are set in the backend when the server boots. As such we need this to be prefixed dynamically which means the frontend is pretty much our only choice

Why do we need this feature?

Fixes a failed plugin loading bug with the Image Renderer plugin for instances of Grafana where requests are served from a sub path via proxy.

Who is this feature for?

Grafana Users

Which issue(s) does this PR fix?:

Fixes #76180

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@wbrowne wbrowne added this to the 10.4.x milestone Jan 31, 2024
@wbrowne wbrowne self-assigned this Jan 31, 2024
Copy link
Contributor

This PR must be merged before a backport PR will be created.

2 similar comments
Copy link
Contributor

This PR must be merged before a backport PR will be created.

Copy link
Contributor

This PR must be merged before a backport PR will be created.

@jackw jackw marked this pull request as ready for review February 7, 2024 09:29
@jackw jackw requested review from diegommm, suntala, idafurjes and academo and removed request for a team February 7, 2024 09:29
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM looks reasonable 👍 Would like some additional eyes on the frontend changes :)

@jackw jackw self-assigned this Feb 7, 2024
@wbrowne wbrowne merged commit 99feb92 into main Feb 8, 2024
14 checks passed
@wbrowne wbrowne deleted the plugin-paths-proxy-rendering-fix branch February 8, 2024 11:19
grafana-delivery-bot bot pushed a commit that referenced this pull request Feb 8, 2024
* don't prepend app sub url to paths

* simplify logo path

* fix(plugins): dynamically prepend appSubUrl for System module resolving to work

* fix(sandbox): support dynamic appSuburl prepend when loading plugin module.js

* fix tests

* update test name

* fix tests

* update fe + add some tests

* refactor(plugins): move wrangleurl to utils, rename to resolveModulePath, update usage

* chore: fix a typo

* test(plugins): add missing name to utils test

* reset test flag

---------

Co-authored-by: Jack Westbrook <jack.westbrook@gmail.com>
(cherry picked from commit 99feb92)
grafana-delivery-bot bot pushed a commit that referenced this pull request Feb 8, 2024
* don't prepend app sub url to paths

* simplify logo path

* fix(plugins): dynamically prepend appSubUrl for System module resolving to work

* fix(sandbox): support dynamic appSuburl prepend when loading plugin module.js

* fix tests

* update test name

* fix tests

* update fe + add some tests

* refactor(plugins): move wrangleurl to utils, rename to resolveModulePath, update usage

* chore: fix a typo

* test(plugins): add missing name to utils test

* reset test flag

---------

Co-authored-by: Jack Westbrook <jack.westbrook@gmail.com>
(cherry picked from commit 99feb92)
wbrowne added a commit that referenced this pull request Feb 8, 2024
…hs (#82146)

Plugins: Don't auto prepend app sub url to plugin asset paths (#81658)

* don't prepend app sub url to paths

* simplify logo path

* fix(plugins): dynamically prepend appSubUrl for System module resolving to work

* fix(sandbox): support dynamic appSuburl prepend when loading plugin module.js

* fix tests

* update test name

* fix tests

* update fe + add some tests

* refactor(plugins): move wrangleurl to utils, rename to resolveModulePath, update usage

* chore: fix a typo

* test(plugins): add missing name to utils test

* reset test flag

---------

Co-authored-by: Jack Westbrook <jack.westbrook@gmail.com>
(cherry picked from commit 99feb92)

Co-authored-by: Will Browne <wbrowne@users.noreply.github.com>
wbrowne added a commit that referenced this pull request Feb 8, 2024
…hs (#82147)

Plugins: Don't auto prepend app sub url to plugin asset paths (#81658)

* don't prepend app sub url to paths

* simplify logo path

* fix(plugins): dynamically prepend appSubUrl for System module resolving to work

* fix(sandbox): support dynamic appSuburl prepend when loading plugin module.js

* fix tests

* update test name

* fix tests

* update fe + add some tests

* refactor(plugins): move wrangleurl to utils, rename to resolveModulePath, update usage

* chore: fix a typo

* test(plugins): add missing name to utils test

* reset test flag

---------

Co-authored-by: Jack Westbrook <jack.westbrook@gmail.com>
(cherry picked from commit 99feb92)

Co-authored-by: Will Browne <wbrowne@users.noreply.github.com>
Ukochka pushed a commit that referenced this pull request Feb 14, 2024
* don't prepend app sub url to paths

* simplify logo path

* fix(plugins): dynamically prepend appSubUrl for System module resolving to work

* fix(sandbox): support dynamic appSuburl prepend when loading plugin module.js

* fix tests

* update test name

* fix tests

* update fe + add some tests

* refactor(plugins): move wrangleurl to utils, rename to resolveModulePath, update usage

* chore: fix a typo

* test(plugins): add missing name to utils test

* reset test flag

---------

Co-authored-by: Jack Westbrook <jack.westbrook@gmail.com>
@aangelisc aangelisc modified the milestones: 10.4.x, 10.4.0 Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Plugins: Bumping SystemJS to 6.14.2 broke image rendering
5 participants