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

Frontend: Detect new assets / versions / config changes #79258

Merged
merged 5 commits into from
Dec 12, 2023
Merged

Frontend: Detect new assets / versions / config changes #79258

merged 5 commits into from
Dec 12, 2023

Conversation

ryantxu
Copy link
Member

@ryantxu ryantxu commented Dec 8, 2023 •
edited
Loading

PR targets #79057

This PR:

  • moves the CDN definition inside the asset path
  • appends CDN prefix to files
  • rather than return the raw values to the frontend, it provides an API with hash values

@ryantxu ryantxu requested review from a team as code owners December 8, 2023 03:44
@ryantxu ryantxu requested review from joshhunt, JoaoSilvaGrafana, sunker, zserge, undef1nd and yangkb09 and removed request for a team December 8, 2023 03:44
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.3.x milestone Dec 8, 2023
dprokop
dprokop reviewed Dec 8, 2023
View reviewed changes
pkg/middleware/recovery.go
@@ -104,7 +105,7 @@ func function(pc uintptr) []byte {

// Recovery returns a middleware that recovers from any panics and writes a 500 if there was one.
// While Martini is in development mode, Recovery will also output the panic as HTML.
func Recovery(cfg *setting.Cfg) web.Middleware {
func Recovery(cfg *setting.Cfg, license licensing.Licensing) web.Middleware {
Copy link
Member

Choose a reason for hiding this comment

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

How's this change related?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, nvm, I see it's required to get the corect CDN path.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right -- but with this approach, I hope we can remove that link. It will be less "safe" because the CDN will not necessarily be the same version, but has more control so the frontend could be ahead of the backend

@academo academo requested a review from xnyo December 8, 2023 09:38
tolzhabayev
tolzhabayev reviewed Dec 11, 2023
View reviewed changes
pkg/api/frontendsettings.go
@@ -25,6 +25,53 @@ import (
"github.com/grafana/grafana/pkg/util"
)

// Returns a file that is easy to check for changes
// Any changes to the file means we should refresh the frontend
Copy link
Contributor

@tolzhabayev tolzhabayev Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

how do we make sure it doesn't result in a endless refresh loop if there are issues? Can we limit the amount of refreshes?

tolzhabayev
tolzhabayev reviewed Dec 11, 2023
View reviewed changes
public/app/core/services/NewFrontendAssetsChecker.ts
const resultRaw = await getBackendSrv().get('/api/frontend/assets');
const result = JSON.stringify(resultRaw);
if (this.checked && this.previous !== result) {
this.hasUpdates = true;
console.log('updates detected', true);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: console.debug maybe?

tolzhabayev
tolzhabayev reviewed Dec 11, 2023
View reviewed changes
public/app/core/services/NewFrontendAssetsChecker.ts
}

public reloadIfUpdateDetected() {
if (this._hasUpdates) {
if (this.hasUpdates) {
window.location.reload();
Copy link
Contributor

@tolzhabayev tolzhabayev Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

didn't we want to do it during next navigation events? Otherwise it will just refresh in the middle of someone editing a dashboard.

Copy link
Member

Choose a reason for hiding this comment

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

@tolzhabayev this function is only called when moving to another page

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, thanks for clarifying.

tolzhabayev
tolzhabayev reviewed Dec 11, 2023
View reviewed changes
public/app/core/services/NewFrontendAssetsChecker.ts
export class NewFrontendAssetsChecker {
private _hasUpdates = false;
private hasUpdates = false;
private previous = '?';
Copy link
Contributor

@tolzhabayev tolzhabayev Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this would mean every grafana instance will have to refresh at least once to fill out the previous state? So basically as soon as you open grafana and navigate for the "first time" (browser session) it will always refresh for everyone?

Copy link
Member Author

Choose a reason for hiding this comment

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

the actual logic makes sure previous has been called before, many ways to slice it, but ths avoids the case you are talking about

if (this.checked && this.previous !== result) {

torkels original version (the PR this is targeting) saves the original values directly in the bootdata -- but I'm not sure we need/want that. This has the downside that the first navigation will never find a change, only the second

torkelo
torkelo approved these changes Dec 12, 2023
View reviewed changes
Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Took this for a spin today, looks good!

@ryantxu ryantxu merged commit 740c45e into detect-new-frontend-assets Dec 12, 2023
12 of 13 checks passed
@ryantxu ryantxu deleted the detect-new-frontend-assets-2 branch December 12, 2023 15:50
@ryantxu ryantxu mentioned this pull request Dec 12, 2023
Merged
torkelo added a commit that referenced this pull request Jan 4, 2024
@torkelo @ryantxu
Frontend: Reload the browser when backend configuration/assets change (…
e924627 
…#79057)

* Detect frontend asset changes

* Update

* merge main

* Frontend: Detect new assets / versions / config changes (#79258)

* avoid first check

* Updates and add tests

* Update

* Update

* Updated code

* refine

* use context

---------

Co-authored-by: Ryan McKinley <ryantxu@gmail.com>
@summerwollin summerwollin modified the milestones: 10.3.x, 10.3.0 Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tolzhabayev tolzhabayev tolzhabayev left review comments

@dprokop dprokop dprokop left review comments

@torkelo torkelo torkelo approved these changes

@joshhunt joshhunt Awaiting requested review from joshhunt joshhunt was automatically assigned from grafana/grafana-frontend-platform

@JoaoSilvaGrafana JoaoSilvaGrafana Awaiting requested review from JoaoSilvaGrafana JoaoSilvaGrafana was automatically assigned from grafana/grafana-frontend-platform

@sunker sunker Awaiting requested review from sunker sunker was automatically assigned from grafana/plugins-platform-frontend

@zserge zserge Awaiting requested review from zserge zserge was automatically assigned from grafana/backend-platform

@undef1nd undef1nd Awaiting requested review from undef1nd undef1nd was automatically assigned from grafana/backend-platform

@yangkb09 yangkb09 Awaiting requested review from yangkb09 yangkb09 was automatically assigned from grafana/backend-platform

@xnyo xnyo Awaiting requested review from xnyo

Assignees
No one assigned
Labels
add to changelog area/backend area/frontend
Projects
None yet
Milestone
10.3.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ryantxu @torkelo @tolzhabayev @dprokop @summerwollin