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

feat: support validate requires version for plugin #3114

Merged
merged 6 commits into from
Jan 13, 2023

Conversation

guqing
Copy link
Member

@guqing guqing commented Jan 7, 2023

What type of PR is this?

/kind feature
/area core
/milestone 2.2.x

What this PR does / why we need it:

插件安装和升级支持版本校验

BTW: 此 PR 中 PluginReconciler 有一些异常提示是没有加 i18n 的,主要是考虑 Reconciler 与请求不挂钩,无法获取到 request 上下文的 Locale,如果用 Locale.getDefault() 那么后续用户切换语言时也更改不到已经持久化到数据库中的错误信息,可能得靠客户端翻译异常。

参考文档:

Which issue(s) this PR fixes:

Fixes #3089

Special notes for your reviewer:

how to test it?

  • 开发模式下不会校验插件填写的 requires,但通过接口安装和升级都会统一校验。
  • 在 deployment 模式下安装插件和升级插件会根据 halo 的版本校验插件的 spec.requires 是否符合要求,参考 semver-expressions-api-ranges
  • 如果 spec.requires 为 * 则表示允许所有,如果填写为具体的版本号,例如 requires: "2.2.0" 则隐式表示为 >=2.2.0

可以测试这几种情况是否符合期望。

/cc @halo-dev/sig-halo

Does this PR introduce a user-facing change?

插件安装和升级支持版本校验

@f2c-ci-robot f2c-ci-robot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Jan 7, 2023
@f2c-ci-robot f2c-ci-robot bot added this to the 2.2.x milestone Jan 7, 2023
@f2c-ci-robot f2c-ci-robot bot added the area/core Issues or PRs related to the Halo Core label Jan 7, 2023
@longjuan
Copy link
Member

longjuan commented Jan 8, 2023

        requires = ">=2.2.0";
        systemVersion = "2.2.0-SNAPSHOT";
        // 这里结果是false
        result = VersionUtils.satisfiesRequires(systemVersion, requires);
        assertThat(result).isTrue();

image

-SNAPSHOT版本好像不行?是否应该考虑在>=2.2.02.2.0-SNAPSHOT版本也通过?

@guqing
Copy link
Member Author

guqing commented Jan 8, 2023

        requires = ">=2.2.0";

        systemVersion = "2.2.0-SNAPSHOT";

        // 这里结果是false

        result = VersionUtils.satisfiesRequires(systemVersion, requires);

        assertThat(result).isTrue();

image

-SNAPSHOT版本好像不行?是否应该考虑在>=2.2.02.2.0-SNAPSHOT版本也通过?

jsemver 表达式的BNF范式是不支持 range expression 写SNAPSHOT的,所以支持SNAPSHOT应该本身就不是一个合理的操作。
zafarkhaja/jsemver#1

开发模式下是不会验证版本的,而生产模式下也不存在 requires 需要写SNAPSHOT的情况,那么 requires 支持 SNAPSHOT 也就失去了意义。

对于

requires = ">=2.2.0";
systemVersion = "2.2.0-SNAPSHOT";

这种情况,系统版本是 SNAPSHOT 而 requires 是大于2.2.0 正式版 所以期望是 false 也没有问题,正式版就是比 SNAPSHOT 大。

@JohnNiang
Copy link
Member

JohnNiang commented Jan 10, 2023

IMO,2.2.0-SNAPSHOT 应当对待成 2.2.0,这样方便在发版前进行插件兼容性测试。

既然正式版中不会包含 SNAPSHOT,那我们如何处理 SNAPSHOT 实际上不会对正式版产生任何影响。

目前 main 分支只要有 commit,我们就会构建出一个 SNAPSHOT 镜像,如果 2.2.0 > 2.2.0-SNAPSHOT 的话,可能无法安装最新的插件进行测试了。

@guqing
Copy link
Member Author

guqing commented Jan 10, 2023

目前 main 分支只要有 commit,我们就会构建出一个 SNAPSHOT 镜像,如果 2.2.0 > 2.2.0-SNAPSHOT 的话,可能无法安装最新的插件进行测试了。

如果 systemVersion endsWith SNAPSHOT 就将其当作正式版使用吗即 2.2.0-SNAPSHOT 提升为 2.2.0 再比较。如果是在本地通过安装最新的 jar 来使用这种场景,确实需要忽略掉 SNAPSHOT

@guqing guqing requested a review from JohnNiang January 10, 2023 07:36
# Conflicts:
#	src/main/java/run/halo/app/plugin/PluginAutoConfiguration.java
@guqing
Copy link
Member Author

guqing commented Jan 11, 2023

/ping @halo-dev/sig-halo

Copy link
Member

@ruibaby ruibaby left a comment

Choose a reason for hiding this comment

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

image

image

image

/lgtm

但是

此 PR 中 PluginReconciler 有一些异常提示是没有加 i18n 的,主要是考虑 Reconciler 与请求不挂钩,无法获取到 request 上下文的 Locale,如果用 Locale.getDefault() 那么后续用户切换语言时也更改不到已经持久化到数据库中的错误信息,可能得靠客户端翻译异常。

不是很理解,#3042 这种方式不能使用吗?

@f2c-ci-robot f2c-ci-robot bot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2023
@guqing
Copy link
Member Author

guqing commented Jan 12, 2023

不是很理解,#3042 这种方式不能使用吗?

endpoint 的异常可以国际化,reconciler 的异常不行,我描述的是 reconciler 的异常需要前端翻译比如插件启动失败,依赖版本检查未通过(升级了 halo,然后已经安装的插件版本不合适了)等。
#3114 (comment) 我处理一下这个,这样使用 jar 安装和升级的时候就可以提示了。

@JohnNiang
Copy link
Member

不是很理解,#3042 这种方式不能使用吗?

endpoint 的异常可以国际化,reconciler 的异常不行,我描述的是 reconciler 的异常需要前端翻译比如插件启动失败,依赖版本检查未通过(升级了 halo,然后已经安装的插件版本不合适了)等。 #3114 (comment) 我处理一下这个,这样使用 jar 安装和升级的时候就可以提示了。

Reconciler 的异常完全没必要国际化。在页面上是无法看到 Reconciler 所抛出的异常。

@JohnNiang
Copy link
Member

我有一个问题:检查版本的时机具体是在”安装“的时候还是在”启用“的时候呢?

如果一个旧插件(此前已经启用)不兼容新版的 Halo 了,那么该插件的状态应该是什么呢?

@guqing
Copy link
Member Author

guqing commented Jan 12, 2023

我有一个问题:检查版本的时机具体是在”安装“的时候还是在”启用“的时候呢?

如果一个旧插件(此前已经启用)不兼容新版的 Halo 了,那么该插件的状态应该是什么呢?

对于 Halo 来说 Reconciler 会将其标记为失败的 phase,然后不会去启动它。
对于 PluginManager 来说处于无法启动的状态,p4fj 将此插件标记为一个中间态 Disable,表示不可启动。

安装和升级的时候都会检查版本,但是升级了 Halo 后这个插件不兼容了,则 Halo 启动时 PluginReconciler 会检查版本是否兼容等启动条件满足时才会启动。

@guqing
Copy link
Member Author

guqing commented Jan 12, 2023

不是很理解,#3042 这种方式不能使用吗?

endpoint 的异常可以国际化,reconciler 的异常不行,我描述的是 reconciler 的异常需要前端翻译比如插件启动失败,依赖版本检查未通过(升级了 halo,然后已经安装的插件版本不合适了)等。 #3114 (comment) 我处理一下这个,这样使用 jar 安装和升级的时候就可以提示了。

Reconciler 的异常完全没必要国际化。在页面上是无法看到 Reconciler 所抛出的异常。

插件启动失败的时候是会显示启动失败原因的

@f2c-ci-robot f2c-ci-robot bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2023
@guqing
Copy link
Member Author

guqing commented Jan 12, 2023

安装和升级时校验的异常已添加国际化支持
71b2e21

image

@guqing guqing requested review from JohnNiang and ruibaby January 12, 2023 15:46
Copy link
Member

@JohnNiang JohnNiang left a comment

Choose a reason for hiding this comment

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

/lgtm

@f2c-ci-robot f2c-ci-robot bot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2023
@f2c-ci-robot
Copy link

f2c-ci-robot bot commented Jan 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JohnNiang

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

The pull request process is described 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

@f2c-ci-robot f2c-ci-robot bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 13, 2023
@f2c-ci-robot f2c-ci-robot bot merged commit 5c29ab5 into halo-dev:main Jan 13, 2023
@guqing guqing deleted the feature/3089 branch January 13, 2023 02:52
@ruibaby ruibaby modified the milestones: 2.2.x, 2.2.0 Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/core Issues or PRs related to the Halo Core kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

插件安装和升级支持版本校验
4 participants