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

Fix HarvesterSchema #37

Closed
wants to merge 2 commits into from
Closed

Fix HarvesterSchema #37

wants to merge 2 commits into from

Conversation

torchiaf
Copy link
Collaborator

@torchiaf torchiaf commented Nov 14, 2024 •
edited
Loading

Summary

  • The error's origin (see Fix HarvesterSchema error #36 ) is the pkg/harvester/models/harvester/schema.js file.
  • When the extension is built locally, linking the shell using yarn link, there is no error (no need to call SteveSchema.reset()).
  • The bug can't be replicated running the extension locally, only by prod build installation.
  • This change needs to be discussed with the team.

PR Checklists

  • Do we need to backport this PR change to the Harvester Dashboard?
    • Yes, the relevant PR is at:
  • Are backend engineers aware of UI changes?
    • Yes, the backend owner is:

Related Issue harvester/harvester#7007

@torchiaf torchiaf requested a review from a110605 November 14, 2024 23:16
@torchiaf torchiaf changed the title Fix/harvester schema 2 Fix HarvesterSchema Nov 14, 2024
@torchiaf torchiaf force-pushed the fix/harvester-schema-2 branch from 33ed717 to 53cebba Compare November 14, 2024 23:22
@torchiaf
Reset Steve Schema
780b258 
Signed-off-by: Francesco Torchia <[email protected]>
@torchiaf torchiaf force-pushed the fix/harvester-schema-2 branch from 53cebba to 780b258 Compare November 15, 2024 00:14
richard-cox
richard-cox reviewed Nov 15, 2024
View reviewed changes
pkg/harvester/store/harvester-store/index.ts
const harvesterFactory = (): CoreStoreSpecifics => {
const harvesterFactory = (config): CoreStoreSpecifics => {
SteveSchema.reset(config.namespace);

const steveFactory = SteveFactory(null, null);
Copy link

Choose a reason for hiding this comment

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

this doesn't look right. the first arg is namespace which i think is important?

Copy link

Choose a reason for hiding this comment

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

The fix from https://github.com/harvester/harvester-ui-extension/pull/36/files looks a bit scary too. That will overwrite the core schema for all stores without it being in the harvester folder.

Could be this line breaking it, given the namespace is missing it's not using /schema?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tested it adding the config param to this code, but I still have the bug. I'm going to close this PR since it's not actually fixing the issue.

richard-cox
richard-cox reviewed Nov 15, 2024
View reviewed changes
pkg/harvester/store/harvester-store/index.ts
import { SteveFactory, steveStoreInit } from '@shell/plugins/steve/index';
import { PRODUCT_NAME } from '../../config/harvester';
import getters from './getters';
import mutations from './mutations';
import actions from './actions';

const harvesterFactory = (): CoreStoreSpecifics => {
const harvesterFactory = (config): CoreStoreSpecifics => {
SteveSchema.reset(config.namespace);
Copy link

Choose a reason for hiding this comment

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

this shouldn't be needed. this store inherits the steve mutation reset which does this

@torchiaf
Copy link
Collaborator Author

@richard-cox I'm closing this PR since I still get the error.
Confirming that, the UI works if we remove the pkg/harvester/models/harvester/schema.js file, but we lose the schemaDefinitions.

@torchiaf torchiaf closed this Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richard-cox richard-cox richard-cox left review comments

@a110605 a110605 Awaiting requested review from a110605

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@torchiaf @richard-cox