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

[chore][scehams] - make it a go pkg #40

Closed
wants to merge 1 commit into from

Conversation

VihasMakwana
Copy link
Member

This will be consumed by meshery/server once released

Notes for Reviewers

This PR fixes #

Signed commits

  • Yes, I signed my commits.

Copy link

welcome bot commented Dec 24, 2023

Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, you can find updates in the #github-notifications channel in the community Slack.
Be sure to double-check that you have signed your commits. Here are instructions for making signing an implicit activity while performing a commit.

@VihasMakwana
Copy link
Member Author

@leecalcote @theBeginner86 is this what you guys are expecting? Let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Can we make this meshery/schemas?

Copy link
Contributor

@MUzairS15 MUzairS15 left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@Yashsharma1911 Yashsharma1911 left a comment

Choose a reason for hiding this comment

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

@VihasMakwana sounds good, however you might not need to import these schemas we already have them here inside /schemas directory here is one of example https://github.com/meshery/schemas/blob/master/schemas/external/ui/catalog/applicationImport.json

A proper structure to store schemas is needed, you might like to check schemas directory in the root directory, the folder structure of schemas in Meshkit isn't good

Copy link
Contributor

@MUzairS15 MUzairS15 left a comment

Choose a reason for hiding this comment

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

Just to remind, this repository is meant to store schemas and their equivalent Golang structs, not any core logic around it. Hence I prefer to keep the logic to refer these RJSF schemas (required by UI) in meshkit, i.e. update the function to refer the schemas defined in this schemas repo using a git walker.

As an init function in a go routine, the RJSF schemas will be imported from this repo (can be lazy loaded as well), and can be served by the Meshery server.

@VihasMakwana
Copy link
Member Author

Will you also open a PR in meshkit to remove the copy of this code? https://github.com/meshery/meshkit/blob/0878150d8145d0cace27e600c44dc66adda961b0/schemas/schemaProvider.go#L32

Yup

this will be consumed by meshery/server once released

Signed-off-by: Vihas Makwana <[email protected]>
@leecalcote
Copy link
Member

There has been another commit or two made. Ready for a second review?

@VihasMakwana
Copy link
Member Author

VihasMakwana commented Dec 30, 2023

@leecalcote @MUzairS15 yeah, PTAL. Thanks

Copy link

stale bot commented Feb 9, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the issue/stale Issue has not had any activity for an extended period of time label Feb 9, 2024
Copy link
Contributor

@MUzairS15 MUzairS15 left a comment

Choose a reason for hiding this comment

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

Thank you for volunteering @VihasMakwana , we have decided to move forward with otherapproach. The RJSF schemas that were being added as a package will now be served form the sistent NPM package.

@leecalcote
Copy link
Member

Given this I'll propose that we close this PR. At the same time, I'll say: @VihasMakwana, thanks for this PR! Come back. We miss you. 😉

@stale stale bot removed the issue/stale Issue has not had any activity for an extended period of time label Feb 9, 2024
@MUzairS15 MUzairS15 closed this Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/chore Necessary task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants