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

fix: not clearing the template engine cache after upgrading the theme #2970

Merged
merged 2 commits into from
Dec 19, 2022

Conversation

minliacom
Copy link
Contributor

@minliacom minliacom commented Dec 15, 2022

What type of PR is this?

/kind improvement

What this PR does / why we need it:

通过在模板引擎管理器里添加clearCache方法,在升级主题后进行缓存刷新,让新模板内容生效。

Which issue(s) this PR fixes:

Fixes #2953

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

@f2c-ci-robot f2c-ci-robot bot added release-note-none Denotes a PR that doesn't merit a release note. kind/improvement Categorizes issue or PR as related to a improvement. labels Dec 15, 2022
@CLAassistant
Copy link

CLAassistant commented Dec 15, 2022

CLA assistant check
All committers have signed the CLA.

@ruibaby
Copy link
Member

ruibaby commented Dec 15, 2022

感谢 @minliacom 的贡献,在 Review 前请先签署我们的 CLA 协议:#2970 (comment)

另外,测试似乎没有通过。

@minliacom minliacom requested review from ruibaby and removed request for wan92hen and lan-yonghui December 15, 2022 12:52
@ruibaby ruibaby changed the title fix: when the theme was upgraded, the template content was not refreshed fix: not clearing the template engine cache after upgrading the theme Dec 15, 2022
@guqing
Copy link
Member

guqing commented Dec 16, 2022

我忽略了一点 卸载的时候应该要在 reconciler 清除缓存,需要在 ThemeResolver 中添加一个根据 theme name 得到 ThemeContext 的方法

@minliacom
Copy link
Contributor Author

我忽略了一点 卸载的时候应该要在 reconciler 清除缓存,需要在 ThemeResolver 中添加一个根据 theme name 得到 ThemeContext 的方法

嗯,我按这样尝试一下

@JohnNiang
Copy link
Member

JohnNiang commented Dec 16, 2022

如果要更细致的话,在切换主题的时候也得清理 Thymeleaf 缓存。

@JohnNiang
Copy link
Member

Hi @minliacom ,建议当前 PR 只解决升级时清理缓存即可。暂时不用管删除和切换主题的情况。

@minliacom
Copy link
Contributor Author

Hi @minliacom ,建议当前 PR 只解决升级时清理缓存即可。暂时不用管删除和切换主题的情况。

那我先把删除和切换的代码删除了再提交

Copy link
Member

@guqing guqing left a comment

Choose a reason for hiding this comment

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

/approve
经测试符合预期 🎉

@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 Dec 16, 2022
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.

/lgtm

@f2c-ci-robot f2c-ci-robot bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2022
@JohnNiang
Copy link
Member

/hold

Please wait for my code review.

@f2c-ci-robot f2c-ci-robot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2022
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.

/unhold

@f2c-ci-robot f2c-ci-robot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2022
@ruibaby
Copy link
Member

ruibaby commented Dec 17, 2022

#2970 (comment)

这里有一个名为 Your Name 的用户名没有签署 CLA,导致无法合并。建议进行以下操作:

git fetch upstream/main

git reset --soft upstream/main

git add .

git commit -m "fix: not clearing the template engine cache after upgrading the theme"

git push -f

@f2c-ci-robot f2c-ci-robot bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2022
@f2c-ci-robot f2c-ci-robot bot added the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Dec 17, 2022
@minliacom
Copy link
Contributor Author

@ruibaby OK了

@f2c-ci-robot f2c-ci-robot bot removed the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Dec 17, 2022
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 Dec 19, 2022
@f2c-ci-robot
Copy link

f2c-ci-robot bot commented Dec 19, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guqing, 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 merged commit efc940d into halo-dev:main Dec 19, 2022
@ruibaby
Copy link
Member

ruibaby commented Dec 19, 2022

@halo-dev/sig-halo 其他需要清理缓存的情况是否需要提交 issue 呢?

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. kind/improvement Categorizes issue or PR as related to a improvement. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

生产环境更新主题没有清理掉模板缓存
5 participants