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

SKS-2530: Support contextual logging #175

Merged
merged 1 commit into from
Apr 12, 2024
Merged

SKS-2530: Support contextual logging #175

merged 1 commit into from
Apr 12, 2024

Conversation

haijianyang
Copy link
Contributor

Issue

当前 CAPE 的 context 使用不规范,例如:

Change

  • 日志使用 CAPI 社区推荐的日志规范
  • controller 传递 controller-runtime 传递过来的 ctx,并取消原有的自定义 Context,不再继承原生的 Context

Test

E2E

@@ -29,7 +30,6 @@ import (

// ClusterContext is a Go context used with a ElfCluster.
type ClusterContext struct {
*ControllerContext
Copy link
Contributor

Choose a reason for hiding this comment

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

ClusterContext 和 MachineContext 都不包含 context.Context 了,考不考虑换个命名,避免和 context 混淆。
函数调用时 r.reconcileNormal(ctx, clusterCtx) 看起来像传了 2 个 context。
我写第一版 cap-ksph 是用过 ClusterScope 这样的命名(不过最后和 context 融合了)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scope 描述不是很准确。

Scope 指的是变量或对象的可见范围,即代码中哪些部分可以访问该变量或对象。在编程语言中,Scope 通常由嵌套的代码块来定义,例如函数、循环、类等。

Context 指的是执行环境,即代码运行时的环境信息。Context 可以包含各种信息,例如当前用户、当前位置、数据库连接等。

两者的主要区别在于:

Scope 是静态的,由代码结构决定,在编译或解释时就已经确定。Context 是动态的,可能会在代码运行时发生变化。
Scope 主要用于控制变量或对象的访问权限,而 Context 主要用于提供执行环境信息。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里可以讨论一下,用什么名字。capv 继续保持使用 ctx,capi 也有在用 scope。

Copy link
Contributor

Choose a reason for hiding this comment

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

capv 也是有 2 个 ctx 么?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这两个 ctx 是没有办法整合到一起了,因为原生的 ctx 按照规范会在使用的过程变动。

Copy link
Contributor

Choose a reason for hiding this comment

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

确实,过程中有可能需要 WithXXX 出一个新的 ctx,我也不希望用原来整到一起的用法。
暂时想不出啥好名字,我觉得可以保持这样

Copy link
Contributor Author

Choose a reason for hiding this comment

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

也许可以尝试一下:把自定义的 ClusterContext/MachineContext 这些保存到 ctx 中。类似于 log 把一些信息写入到 ctx 中,并可以从中读取,但我们的场景会改动 cluster/machine,不知道有没有影响。

@haijianyang haijianyang merged commit 2f0e706 into master Apr 12, 2024
1 check passed
@haijianyang haijianyang deleted the context branch April 12, 2024 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants