Skip to content

frontend: PluginSettings: Refactor local storage and plugin data #3123

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vyncent-t
Copy link
Contributor

original - #2671

Fixes Issue #2595

Description

  • This PR shrinks the saved JSON data from the plugins information and saves only the name and isEnabled status in local storage. This ensures plugin settings persist when closing and reopening the app.
  • This PR introduces the usePlugins hook into lib/k8s/api/v2
  • It also handles backward compatibility, allowing previously saved settings to be used in this new format.
  • Additionally, it no longer takes old information from the JSON in local storage. Instead, it retrieves the necessary plugin data from the backend when the main Plugin component is called.

Changes

  1. Introduces usePlugins hook: Hook used to fetch the plugins from the backend and return the plugins with their settings
  2. Reduced Storage Data: Only stores essential plugin info (name and isEnabled) in local storage.
  3. Backward Compatibility: Converts old-format data into the new structure on first run.
  4. Data Source Shift: Eliminates use of outdated local storage JSON data, and instead fetches plugin details from the backend when the Plugin component mounts.

How to Test

  1. Setup:
    • Pull this branch and run the application.
  2. Verify Plugin Settings:
    • Open headlamp in app mode
    • Navigate to the settings page then to plugins settings tab
    • Check local storage item headlampPluginSettings
  3. Check Backward Compatibility:
    • Start with an older version of the app or start from main.
    • Navigate to the settings page then to plugins settings tab
    • Check local storage item headlampPluginSettings
    • Switch to this branch and run the app.
    • Confirm that previously stored plugin settings are recognized and converted correctly.
  4. Backend Data:
    • Verify that after loading, the plugin details (other than name and isEnabled) are fetched from the backend and displayed correctly in the UI.
    • Switch plugins off and on and observe the changes in local storage and in app

Notes

  • If you have preexisting local storage data, this PR will convert it automatically. You can reset the local storage if you return to main and go to the same plugin settings paage.
  • Double-check the logs to ensure no errors appear during the migration from old data to new data.

@vyncent-t vyncent-t self-assigned this Apr 15, 2025
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 15, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vyncent-t
Once this PR has been reviewed and has the lgtm label, please assign illume for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 15, 2025
@vyncent-t vyncent-t marked this pull request as ready for review April 15, 2025 20:06
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 15, 2025
@k8s-ci-robot k8s-ci-robot requested review from ashu8912 and illume April 15, 2025 20:06
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 18, 2025
@vyncent-t vyncent-t changed the title [NEW] frontend: PluginSettings: Refactor local storage and plugin data frontend: PluginSettings: Refactor local storage and plugin data Apr 18, 2025
@vyncent-t vyncent-t force-pushed the reduce-plugin-settings-data branch 2 times, most recently from de2cb34 to 9c04cce Compare April 22, 2025 17:54
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2025
@vyncent-t vyncent-t force-pushed the reduce-plugin-settings-data branch 3 times, most recently from c1fb342 to e995299 Compare April 22, 2025 19:05
@vyncent-t vyncent-t force-pushed the reduce-plugin-settings-data branch 2 times, most recently from 7e6f2f5 to d23b279 Compare April 23, 2025 16:50
@vyncent-t
Copy link
Contributor Author

e2e tests are passing, none of the log changes cause it to fail. going to drop e2e test changes here and rebase when #3144 is merged

@vyncent-t vyncent-t force-pushed the reduce-plugin-settings-data branch 2 times, most recently from 1adc26f to 52bc764 Compare April 25, 2025 18:31
Copy link
Contributor

@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.

I'm against moving the logic around into more places. It makes it more difficult to hunt for things.

Additionally, there are a bunch of separate changes all together in one commit.

@vyncent-t vyncent-t force-pushed the reduce-plugin-settings-data branch from 52bc764 to 9b5e04a Compare April 28, 2025 16:49
@vyncent-t
Copy link
Contributor Author

I'm against moving the logic around into more places. It makes it more difficult to hunt for things.

Additionally, there are a bunch of separate changes all together in one commit.

I have another PR that requires these changes so that the plugin info is compatible with plugins settings headlamp-k8s/plugins#117

@vyncent-t vyncent-t force-pushed the reduce-plugin-settings-data branch from 9b5e04a to f432e02 Compare May 7, 2025 14:32
@vyncent-t vyncent-t force-pushed the reduce-plugin-settings-data branch from f432e02 to 945c9c9 Compare May 7, 2025 14:41
@vyncent-t vyncent-t force-pushed the reduce-plugin-settings-data branch from 945c9c9 to a624f0a Compare May 19, 2025 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants