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: plugin compatibility infos #966

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

alfredotoselli
Copy link

Description

Added min and max version on plugin manifest.
If we agree on the structure I'll update admin-vue and ts-client (Plugin.ts type)

We want to enforce it? e.g. on plugin installation or activation (Not necessary for me)

Related to issue #960

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas

@lucagobbi
Copy link
Collaborator

I wouldnt enforce it for the moment, maybe a warning in the admin or in the logs?

@pieroit
Copy link
Member

pieroit commented Nov 7, 2024

No enforcing, will make things way more complicated

Copy link
Member

@pieroit pieroit left a comment

Choose a reason for hiding this comment

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

Thanks @alfredotoselli left some change request

# Core compatibility
compatibility = json_file_data.get(
"compatibility",
{
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the structure flat for the moment, maybe with a more explicit naming, i.e.
version, ..., min_cat_version, max_cat_version

"compatibility",
{
"min_version": "not specified",
"max_version": "not specified",
Copy link
Member

Choose a reason for hiding this comment

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

maybe empty string as default value, we made a mess in the past with a different default for every key XD

Copy link
Author

@alfredotoselli alfredotoselli Nov 7, 2024

Choose a reason for hiding this comment

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

agree, also easier to handle in admin-vue

@alfredotoselli
Copy link
Author

alfredotoselli commented Nov 7, 2024

Here is admin-vue PR

About an eventual warning we could:

  • check the versions on get_available_plugins API, log a warning and return the info to be used by admin-vue
  • check the version somewhere else in BE, just log a warning
  • check the version at FE, showing a warning only in admin vue
  • not doing it at all

@valentimarco valentimarco added the enhancement New feature or request label Nov 7, 2024
@pieroit pieroit merged commit c331141 into cheshire-cat-ai:develop Nov 13, 2024
2 checks passed
@pieroit
Copy link
Member

pieroit commented Nov 13, 2024

Here is admin-vue PR

About an eventual warning we could:

  • check the versions on get_available_plugins API, log a warning and return the info to be used by admin-vue
  • check the version somewhere else in BE, just log a warning
  • check the version at FE, showing a warning only in admin vue
  • not doing it at all

Thanks @alfredotoselli, just merged.

Let's see what the contribs think about the strategy here.
Any take of responsibility on plugin compatibility is going to cost maintaining, so my preference is on waiting to better focus and with some feedback at hand

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants