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

headlamp-plugin: Make update check asynchronous #1941

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

skoeva
Copy link
Contributor

@skoeva skoeva commented May 1, 2024

This change makes the update check for headlamp-plugin asynchronous to avoid slowing down and blocking the user on start.

Fixes: #1899

@skoeva skoeva linked an issue May 1, 2024 that may be closed by this pull request
@skoeva skoeva self-assigned this May 2, 2024
@skoeva skoeva force-pushed the speed-up-headlamp-plugin branch 8 times, most recently from 431b6aa to 8739392 Compare June 1, 2024 01:11
@skoeva skoeva marked this pull request as ready for review June 1, 2024 01:12
@skoeva skoeva force-pushed the speed-up-headlamp-plugin branch from 8739392 to 2286cf2 Compare June 1, 2024 01:14
@skoeva skoeva requested a review from illume June 1, 2024 01:14
Copy link
Collaborator

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

Besides the comments about the logic. I think this PR is doing 2 things: replacing the function that checks for the latest pkg version + limiting the checks.

plugins/headlamp-plugin/bin/headlamp-plugin.js Outdated Show resolved Hide resolved
plugins/headlamp-plugin/bin/headlamp-plugin.js Outdated Show resolved Hide resolved
plugins/headlamp-plugin/bin/headlamp-plugin.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

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

From this comment in the issue: #1899 (comment)

It could be made async, so it doesn't block the user getting work done.

If the check is done async, then it can show the message some moments later. This avoids having state, and has the benefit that it isn't slow once a day. The user is never slowed down in their work.

Currently doing npm start with this branch will do the check in an async fashion never block the user. So I think all of the last check stuff is not necessary.

The main goal of not blocking the user is achieved with the code:

  informIfOutdated().catch(error => {
    console.error('Failed to check for updates:', error);
  });

So please keep that informIfOutdated() call, and remove all the lastCheckTime code and the force-check code.

@illume illume marked this pull request as draft June 19, 2024 11:36
@skoeva skoeva force-pushed the speed-up-headlamp-plugin branch 2 times, most recently from 8b7c61a to 69cb257 Compare June 19, 2024 15:44
@skoeva skoeva changed the title headlamp-plugin: Limit update checks to daily headlamp-plugin: Make update check asynchronous Jun 19, 2024
@skoeva skoeva force-pushed the speed-up-headlamp-plugin branch 9 times, most recently from b9f62bb to ceba257 Compare June 20, 2024 13:55
@skoeva skoeva requested a review from illume June 20, 2024 14:08
@skoeva skoeva marked this pull request as ready for review June 20, 2024 14:08
@illume illume force-pushed the speed-up-headlamp-plugin branch from b7eedee to 02359a3 Compare June 20, 2024 15:22
@illume
Copy link
Collaborator

illume commented Jun 20, 2024

Hi,

when testing I noticed when using the execSync the user is blocked waiting still for the dependency update check.

So, for your consideration I pushed a fix up commit with some changes and console.debug lines in there so you can see the execution. Because execSync runs synchronously, these changes were needed. Please have a look, and squash the changes into your commit how you like without the console.debug lines if you think this is ok?

How to test, step 1

cd plugins/headlamp-plugin
npm run build && npm pack

cd ../examples/pod-counter
npm i ../../headlamp-plugin/kinvolk-headlamp-plugin-0.9.1.tgz
npm start

In this screen shot we see that the plugin builds right away. The user does not wait for the outdate check to start or finish.
image

testing step 2

Now check it's showing the outdated message:

  • edit package.json and change the
  • npm start
  • it should show that the package is outdated
  "devDependencies": {
    "@kinvolk/headlamp-plugin": "^0.9.0"
  }

In this screen short, we see the user does not need to wait for the dependency check to start or finish. It also shows the message to the user that their package is outdated.
image

Notes:

The setTimeout starts 500ms later, so webpack has some time to begin and get some work done. Because that has priority over the dependency outdated check.

The console.debug lines should be removed, but they are useful to test.

@skoeva
Copy link
Contributor Author

skoeva commented Jun 20, 2024

@illume I see, thanks for the fix! I think this makes sense since I noticed issues with performing the getNpmOutdated() code async, I can remove the debug lines and squash this now

@skoeva skoeva force-pushed the speed-up-headlamp-plugin branch from 02359a3 to 4f9cdbe Compare June 20, 2024 17:23
This change makes the update check for headlamp-plugin asynchronous
to avoid slowing down and blocking the user on start.

Fixes: #1899

Signed-off-by: Evangelos Skopelitis <eskopelitis@microsoft.com>
@skoeva skoeva force-pushed the speed-up-headlamp-plugin branch from 4f9cdbe to 948c4b8 Compare June 20, 2024 17:28
@joaquimrocha joaquimrocha merged commit dc251bb into main Jun 21, 2024
12 checks passed
@joaquimrocha joaquimrocha deleted the speed-up-headlamp-plugin branch June 21, 2024 09:15
@illume illume added plugins headlamp-plugin Related to the headlamp-plugin NPM package. labels Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
headlamp-plugin Related to the headlamp-plugin NPM package. plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make headlamp-plugin faster
3 participants