[CI] add basic static check, complie, test #292

357447923 · 2023-08-17T05:05:32Z

No description provided.

357447923 · 2023-08-17T05:44:52Z

#256

Wine93 · 2023-08-28T01:25:43Z

@357447923 麻烦先尝试让 CI 跑过 :)

357447923 · 2023-08-29T14:40:52Z

@357447923 麻烦先尝试让 CI 跑过 :)

ok，我已经解决了CI不通过的问题了，麻烦您再试试看

Wine93 · 2023-08-30T04:40:12Z

.golangci.yml

+        - gosimple
+      text: "S1009:"
+  # Fix found issues (if it's supported by the linter).
+  fix: true


这里需要一个换行。

这里需要一个换行。

是在最后加一个换行吗？抱歉，我不是很理解这个“需要一个换行”，麻烦您跟我说一下是在哪一行加上。

Wine93 · 2023-08-30T04:41:25Z

cli/cli/cli.go

@@ -143,7 +143,7 @@ func (curveadm *CurveAdm) init() error {
 	if err := log.Init(config.GetLogLevel(), logpath); err != nil {
 		return errno.ERR_INIT_LOGGER_FAILED.E(err)
 	} else {
-		log.Info("Init logger success",
+		_ = log.Info("Init logger success",


Hi @357447923 ，在目前的检查工具里面，赋值这个返回值是必须的是吗？有没有参数可以跳过该强制限制，因为我觉得这样的写法是比较常见的，或者说如果不赋值返回值会带来哪些副作用呢？

Hi @357447923 ，在目前的检查工具里面，赋值这个返回值是必须的是吗？有没有参数可以跳过该强制限制，因为我觉得这样的写法是比较常见的，或者说如果不赋值返回值会带来哪些副作用呢？

按照目前的配置来说，这个赋值返回值是必须的。是有跳过该限制的参数，不过可能得找一下有没有粒度更小的处理方法。我之前加上过一次这个参数，它会把另外的一些问题也进行忽略。
我最近来找一下这个处理方法。

Hi @357447923 ，在目前的检查工具里面，赋值这个返回值是必须的是吗？有没有参数可以跳过该强制限制，因为我觉得这样的写法是比较常见的，或者说如果不赋值返回值会带来哪些副作用呢？

你的意思是把errcheck给关闭还是说只忽略打印日志的这种情况？

Wine93 · 2023-08-30T04:43:28Z

另外值得一提的是，在修改之后，咱们有尝试部署一个 BS/FS 集群并挂载客户端来进行测试吗？

357447923 · 2023-08-30T06:39:13Z

另外值得一提的是，在修改之后，咱们有尝试部署一个 BS/FS 集群并挂载客户端来进行测试吗？

还没有尝试。等我最近的一些急事做完后，我就来进行测试。

github-advanced-security · 2023-09-15T09:50:00Z

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

Wine93 · 2023-09-21T09:22:00Z

cli/command/deploy.go

@@ -223,7 +223,7 @@ func genDeployPlaybook(curveadm *cli.CurveAdm,
 			role := DEPLOY_FILTER_ROLE[step]
 			config = curveadm.FilterDeployConfigByRole(config, role)
 		}
-		n := len(config)
+		var n int


这里的代码逻辑好像改变了，n 没有默认值了，应该是有问题的~

这里的代码逻辑好像改变了，n 没有默认值了，应该是有问题的~

您好，不过我看了下，这个值似乎只在下面的一小段代码块中使用，并且是使用之前会进行赋值。所以ci就提示我说这个赋值冗余

var n int
if DEPLOY_LIMIT_SERVICE[step] > 0 {
n = DEPLOY_LIMIT_SERVICE[step]
config = config[:n]
}

这里的代码逻辑好像改变了，n 没有默认值了，应该是有问题的~

您好，不过我看了下，这个值似乎只在下面的一小段代码块中使用，并且是使用之前会进行赋值。所以ci就提示我说这个赋值冗余

var n int if DEPLOY_LIMIT_SERVICE[step] > 0 { n = DEPLOY_LIMIT_SERVICE[step] config = config[:n] }

那这个 n 是不是可以不用定义，直接赋值~

Wine93 · 2023-09-21T09:23:28Z

internal/playbook/configs.go

 	// multi-configs
 	case []*hosts.HostConfig:
 		c.ctype = TYPE_CONFIG_HOST
-		c.hcs = configs.([]*hosts.HostConfig)
+		c.hcs = configs


这里类型会做自动转换是吗？

这里类型会做自动转换是吗？

把switch configs.(type) 改为 switch configs := configs.(type) 之后，case语句下的configs就自动转为对应类型了

这里类型会做自动转换是吗？

把switch configs.(type) 改为 switch configs := configs.(type) 之后，case语句下的configs就自动转为对应类型了

💯

Wine93 · 2023-09-21T09:23:54Z

internal/playbook/playbook.go

@@ -91,7 +91,7 @@ func (p *Playbook) run(steps []*PlaybookStep) error {
 			return err
 		}

-		isLast := (i == len(steps)-1)
+		isLast := i == len(steps)-1


这里为什么要把括号去掉，加上括号可能更好理解一点~

这里为什么要把括号去掉，加上括号可能更好理解一点~

可能是我误删了，我记得还有好几个类似场景，但是ci是通过的

Wine93 · 2023-09-21T09:24:52Z

internal/task/task/playground/init.go

 	case *topology.DeployConfig:
-		dc := cfg.(*topology.DeployConfig)
+		dc := cfg


Wine93

LGTM!

caoxianfei1 · 2023-09-25T09:26:02Z

.golangci.yml

+      - loopclosure
+
+  staticcheck:
+    checks: ["all", "-ST1000", "-ST1003", "-ST1016", "-ST1020", "-ST1021", "-ST1022", "-SA4006"]


is default value? no need to config here if so.

is default value? no need to config here if so.

Yes, you're right. These configs are redundant. I had already read official document of golangci-lint just now and made ensure my configs was default value. So I think these configs can be removed. Need I remove them?

yes, you can.

yes, you can.

Oh,no. After my test remove them. I recalled why I didn't use the default values directly because some of the checks I think them were not necessary. For example, MD5 cannot be used (because golangci-lint would think it is used for encryption, and it believes that weak encryption should not be used for encryption), but in reality, when we use MD5, it may not necessarily be used for encryption.

So what happend after remove the configuration for our programe.

So what happend after remove the configuration for our programe.

I paste all my local environment error messages here：

internal/task/task/checker/network.go:142:17: G601: Implicit memory aliasing in for loop. (gosec)
Destination: &address,
^
internal/task/task/checker/kernel_test.go:50:32: G601: Implicit memory aliasing in for loop. (gosec)
lambda := checkKernelVersion(&t.version, &dc)
^
internal/utils/common.go:30:2: G501: Blocklisted import crypto/md5: weak cryptographic primitive (gosec)
"crypto/md5"
^
internal/utils/common.go:205:7: G401: Use of weak cryptographic primitive (gosec)
m := md5.New()
^
internal/utils/common.go:220:9: G204: Subprocess launched with a potential tainted input or cmd arguments (gosec)
return exec.Command(args[0], args[1:]...)
^
internal/utils/common.go:242:22: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
b[i] = letterRunes[rand.Intn(len(letterRunes))]
^
cli/command/exec.go:51:4: SA4006: this value of args is never used (staticcheck)
args = args[:1]
^
internal/common/common.go:73:2: G101: Potential hardcoded credentials (gosec)
KEY_CHECK_KERNEL_MODULE_NAME = "CHECK_KERNEL_MODULE_NAME"
^
internal/build/debug.go:34:2: G101: Potential hardcoded credentials (gosec)
DEBUG_CURVEADM_CONFIGURE = "DEBUG_CURVEADM_CONFIGURE"
^
internal/tools/ssh.go:88:9: G204: Subprocess launched with a potential tainted input or cmd arguments (gosec)
return exec.Command(items[0], items[1:]...), nil
^
internal/tools/upgrade.go:105:9: G204: Subprocess launched with a potential tainted input or cmd arguments (gosec)
cmd := exec.Command("/bin/bash", "-c", fmt.Sprintf("curl -fsSL %s | bash", URL_INSTALL_SCRIPT))

So what happend after remove the configuration for our programe.

Could you tell me which problems need to be ignored? I will try to handle those need to fix.

caoxianfei1 · 2023-09-25T09:27:12Z

.golangci.yml

+
+  bidichk:
+    # The following configurations check for all mentioned invisible unicode runes.
+    # All runes are enabled by default.


All runes are enabled by default? So no need config here also?

caoxianfei1 · 2023-09-25T11:19:25Z

.golangci.yml

+    pop-directional-isolate: true
+
+
+issues:


What is the work of issue here?

What is the work of issue here?

Only bidichk can be removed. Removing it has no impact.

caoxianfei1 · 2023-09-25T11:22:54Z

.golangci.yml

+      text: "S1009:"
+
+  # Fix found issues (if it's supported by the linter).
+  fix: true


fix is mean that lint rune can fix problem that already been detected? Is it possible？

fix is mean that lint rune can fix problem that already been detected? Is it possible？

Can only solve a few problems

I think can take a closer look at the configuration options and whether the default configuration is enough. The .golangci.yaml is necessary? for example, gosec and govet why check these other than check all.

I think can take a closer look at the configuration options and whether the default configuration is enough. The .golangci.yaml is necessary? for example, gosec and govet why check these other than check all.

The. golangci.yaml file is required as it is a configuration for golangci-lint. Although golangci-lint has many default configurations, but not every configuration is suitable for all projects. Many default configurations burden developers, such as forcing errcheck, ensure project has no unused functions and variables.

I think can take a closer look at the configuration options and whether the default configuration is enough. The .golangci.yaml is necessary? for example, gosec and govet why check these other than check all.

Using the default configuration is not bad, but it covers nearly everything. It can lead to some issues, such as not being able to use MD5, Rand and also so. If you find all of the Gosec checks acceptable, I will use the default configs.

yes, you are right. We are not familiar with this configuration also, but we can see from the error report above that some errors need to be handled. For example, unused plugin should report as an error or warning, but some may not be necessary. For example, errorcheck plugin in staticcheck may not always be necessary, for gosec, G501, G401 may be etc. You can run once according to default configuration, see which ones will report errors for our program, but which of these errors can be ignored, and then enable or disable them may be more effective.

yes, you are right. We are not familiar with this configuration also, but we can see from the error report above that some errors need to be handled. For example, unused plugin should report as an error or warning, but some may not be necessary. For example, errorcheck plugin in staticcheck may not always be necessary, for gosec, G501, G401 may be etc. You can run once according to default configuration, see which ones will report errors for our program, but which of these errors can be ignored, and then enable or disable them may be more effective.

Good suggestion! I may find more necessary configs after running project and reaing official document. Please waiting for the change of '.golangci.yaml'

caoxianfei1 · 2023-09-26T06:32:57Z

.golangci.yml

+      text: "SA1019:"
+    - linters:
+        - gosimple
+      text: "S1009:"


S1009 means that redundant nil check on slices and seems to belong to staticcheck, but why exclude it? Also, SA1019. we can fix it now if we use deprecated function If it's easy to modify.

S1009 means that redundant nil check on slices and seems to belong to staticcheck, but why exclude it? Also, SA1019. we can fix it now if we use deprecated function If it's easy to modify.

You are right. they don't seem too difficult to modify. I will now attempt to make some changes and see if I can remember why I configured them that way originally

caoxianfei1 · 2023-09-26T09:26:13Z

You can modify the Makefile for example:

build: fmt vet
debug: fmt vet

fmt:
  go fmt ./...

vet:
  go vet ./...

Then we can lookup the problem in compile phase .

caoxianfei1 · 2023-09-27T01:51:53Z

.golangci.yml

+    - bidichk
+  disable:
+    # ignore unused check
+    - unused


If go vet is executed, unused functions or variables will no longer appear here. Enable it or delete it directly.

If go vet is executed, unused functions or variables will no longer appear here. Enable it or delete it directly.

Are you suggesting that I remove the 'unused' configuration and remove unused variables and functions in this project code?

yes, but you seems to have deleted them in code?

yes, but you seems to have deleted them in code?

no, this rule 'unused' considers fields in a struct and functions belonging to a struct as well. So, I'm unsure if I should remove them, as I don't know if they are useful in the future.

I think they can be removed.

I think they can be removed.

Okey, I had removed them just now.

Signed-off-by: xiaoming <[email protected]> [fix]playbook: fix format related variables position Signed-off-by: sjf <[email protected]> [fix] add support for recognizing kernel versions like "3.15.0_.*" Signed-off-by: jyf111 <[email protected]> support enable etcd auth Signed-off-by: wanghai01 <[email protected]> Feature: podman can be an alternative container engine Signed-off-by: caoxianfei1 <[email protected]> bugfix: docker ps specify both --format and --quiet will warning since v24.0.0. WARNING: Ignoring custom format, because both --format and --quiet are set. Signed-off-by: caoxianfei1 <[email protected]> change replicas to instances Signed-off-by: tec-rubbish maker <[email protected]> change topology variable change service_replicas_sequence and format_replicas_sequence to service_instances_sequence and format_instances_sequence Signed-off-by: tec-rubbish maker <[email protected]> fix the worng instances Signed-off-by: tec-rubbish maker <[email protected]> mistake Signed-off-by: tec-rubbish maker <[email protected]> Update cli/command/status.go Signed-off-by: Wine93 <[email protected]> [fix] older versions of the ss command do not have the --no-header option solution: execute the ss command in a temporary container to precheck for ports in use instead Signed-off-by: jyf111 <[email protected]> Signed-off-by: xiaoming <[email protected]>

caoxianfei1 · 2023-10-07T10:34:31Z

lgtm.

Wine93 self-assigned this Aug 28, 2023

Wine93 added this to the v0.3.0 milestone Aug 28, 2023

Wine93 added the merge to v0.3.0 label Aug 28, 2023

357447923 force-pushed the develop branch 2 times, most recently from c0279c7 to aec00ff Compare August 29, 2023 14:31

Wine93 reviewed Aug 30, 2023

View reviewed changes

Wine93 assigned caoxianfei1 Aug 30, 2023

caoxianfei1 added the developer activities label Sep 5, 2023

357447923 force-pushed the develop branch 3 times, most recently from b0a9e4d to 75de135 Compare September 21, 2023 08:46

Wine93 reviewed Sep 21, 2023

View reviewed changes

internal/task/task/playground/init.go

case *topology.DeployConfig:

dc := cfg.(*topology.DeployConfig)

dc := cfg

Copy link

Collaborator

Wine93 Sep 21, 2023

Choose a reason for hiding this comment

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

ditto.

357447923 force-pushed the develop branch 2 times, most recently from 75de135 to 794d342 Compare September 21, 2023 10:21

Wine93 approved these changes Sep 22, 2023

View reviewed changes

caoxianfei1 reviewed Sep 25, 2023

View reviewed changes

caoxianfei1 reviewed Sep 26, 2023

View reviewed changes

357447923 force-pushed the develop branch from 794d342 to 07394ad Compare September 26, 2023 08:32

357447923 force-pushed the develop branch from 07394ad to 836bbc2 Compare September 26, 2023 13:20

caoxianfei1 reviewed Sep 27, 2023

View reviewed changes

357447923 force-pushed the develop branch from 836bbc2 to c6f9757 Compare September 27, 2023 08:28

357447923 force-pushed the develop branch 10 times, most recently from f3fd9f0 to c6f9757 Compare October 7, 2023 10:06

357447923 force-pushed the develop branch from c6f9757 to 33be96f Compare October 7, 2023 10:13

caoxianfei1 approved these changes Oct 7, 2023

View reviewed changes

caoxianfei1 merged commit de8f69d into opencurve:develop Oct 7, 2023
6 checks passed

caoxianfei1 removed the developer activities label Nov 27, 2023

[CI] add basic static check, complie, test #292

[CI] add basic static check, complie, test #292

Conversation

357447923 commented Aug 17, 2023

357447923 commented Aug 17, 2023

Wine93 commented Aug 28, 2023

357447923 commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wine93 commented Aug 30, 2023

357447923 commented Aug 30, 2023

github-advanced-security bot commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wine93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caoxianfei1 Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caoxianfei1 commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caoxianfei1 commented Oct 7, 2023

caoxianfei1 Sep 26, 2023 •

edited

Loading