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

Wrangler CI #270

Merged
merged 3 commits into from
Feb 21, 2023
Merged

Wrangler CI #270

merged 3 commits into from
Feb 21, 2023

Conversation

KevinJoiner
Copy link
Contributor

Issue: #210

Fixed the apply unit test to stop panicking due to the use of an empty schema.

Adds validation and unit test to drone pipeline.

I had to fix the following golangci-lint errors as well.

[test:70] pkg/objectset/objectset.go:116:8: Error return value of `o.err` is not checked (errcheck)
[test:71]               o.err(errors.Wrapf(err, "failed to add %T", obj))
[test:72]                    ^
[test:73] pkg/objectset/objectset.go:122:8: Error return value of `o.err` is not checked (errcheck)
[test:74]               o.err(errors.Wrapf(err, "failed to add %T", obj))
[test:75]                    ^
[test:76] pkg/apply/desiredset_apply.go:147:13: Error return value of `result.Add` is not checked (errcheck)
[test:77]               result.Add(obj)
[test:78]                         ^
[test:79] pkg/apply/desiredset_process.go:213:8: Error return value of `o.err` is not checked (errcheck)
[test:80]               o.err(err)
[test:81]                    ^
[test:85] pkg/generic/factory.go:106:23: Error return value of `c.cacheFactory.Start` is not checked (errcheck)
[test:86]               c.cacheFactory.Start(ctx)
[test:87]                                   ^
[test:88] pkg/relatedresource/changeset.go:86:15: Error return value is not checked (errcheck)
[test:89]                               runResolve(meta.GetNamespace(), meta.GetName(), ro)
[test:90]                                         ^
[test:91] pkg/needacert/needacert.go:260:23: Error return value of `h.locker.Unlock` is not checked (errcheck)
[test:92]       defer h.locker.Unlock(lockKey)
[test:93]                            ^
[test:94] pkg/controller-gen/main.go:33:2: var `t` is unused (unused)
[test:95]       t = true
[test:96]       ^
[test:97] pkg/schemas/mappers/enum.go:15:2: field `field` is unused (unused)
[test:98]       field string
[test:99]       ^
[test:100] pkg/apply/desiredset.go:42:2: field `codeVersion` is unused (unused)
[test:101]      codeVersion              string
[test:102]      ^
[test:103] pkg/schemas/reflection.go:348:4: S1011: should replace loop with `opts = append(opts, strings.Split(parts[1], "|")...)` (gosimple)
[test:104]                      for _, opt := range strings.Split(parts[1], "|") {
[test:105]                      ^
[test:106] pkg/controller-gen/main.go:140:11: S1005: unnecessary assignment to the blank identifier (gosimple)
[test:107]      for pkg, _ := range pathsToCopy {
[test:108]               ^
[test:109] pkg/apply/desiredset_apply.go:92:11: S1012: should use `time.Since` instead of `time.Now().Sub` (gosimple)
[test:110]              if d := time.Now().Sub(t); d.Seconds() > 1 {
[test:111]                      ^
[test:112] pkg/git/git.go:352:48: S1039: unnecessary use of fmt.Sprintf (gosimple)
[test:113]              cmd.Env = append(cmd.Env, "GIT_SSH_COMMAND="+fmt.Sprintf("ssh -o StrictHostKeyChecking=accept-new"))
[test:114]                                                           ^
[test:115] pkg/summary/summarized.go:57:2: S1023: redundant `return` statement (gosimple)
[test:116]      return
[test:117]      ^
[test:118] pkg/summary/summarized.go:81:2: S1023: redundant `return` statement (gosimple)
[test:119]      return
[test:120]      ^
[test:121] pkg/schemas/mappers/move.go:45:28: ineffectual assignment to ok (ineffassign)
[test:122]      toSchema, toFieldName, _, ok, err := getField(s, schemas, m.To)
[test:123]                                ^
[test:124] pkg/condition/condition.go:226:3: SA4006: this value of `t` is never used (staticcheck)
[test:125]              t = v.Type()
[test:126]              ^
[test:127] pkg/controller-gen/main.go:6:2: SA1019: "io/ioutil" has been deprecated since Go 1.16: As of Go 1.16, the same functionality is now provided by package io or package os, and those implementations should be preferred in new code. See the specific function documentation for details. (staticcheck)
[test:128]      "io/ioutil"
[test:129]      ^
[test:130] pkg/git/git.go:10:2: SA1019: "io/ioutil" has been deprecated since Go 1.16: As of Go 1.16, the same functionality is now provided by package io or package os, and those implementations should be preferred in new code. See the specific function documentation for details. (staticcheck)
[test:131]      "io/ioutil"
[test:132]      ^
[test:133] pkg/kubeconfig/loader.go:5:2: SA1019: "io/ioutil" has been deprecated since Go 1.16: As of Go 1.16, the same functionality is now provided by package io or package os, and those implementations should be preferred in new code. See the specific function documentation for details. (staticcheck)
[test:134]      "io/ioutil"
[test:135]      ^
[test:136] pkg/data/convert/convert.go:238:14: SA1019: strings.Title has been deprecated since Go 1.18 and an alternative has been available since Go 1.0: The rule Title uses for word boundaries does not handle Unicode punctuation properly. Use golang.org/x/text/cases instead. (staticcheck)
[test:137]              parts[i] = strings.Title(parts[i])
[test:138]          

.drone.yml Outdated
image: registry.suse.com/bci/golang:1.19
commands:
- zypper -n install tar gzip
- "curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s v1.49.0;"
Copy link
Contributor

Choose a reason for hiding this comment

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

The latest release of golangci-lint is 1.51.1. Why not use a newer version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just pulled this from what Rancher was using I will update it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to do it now but, we should move this to a GitHub Action (one exists). Then we can have dependabot manage the dependency to keep that up to date.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can see what Fleet is doing with this here and here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't spent much time looking into actions. How do you decide what should use actions and what should be validated via drone?

Copy link
Contributor

Choose a reason for hiding this comment

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

Drone MUST be used for builds and for tests that go across multiple architectures. s390x isn't something that GitHub Actions can do.

I think other things are just fine in GitHub Actions. Linting is something that doesn't matter per os/arch, for example.

Copy link
Contributor

@rmweir rmweir left a comment

Choose a reason for hiding this comment

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

Also waiting on golangci-lint update to newer version as mentioned by @mattfarina.

.drone.yml Outdated
image: registry.suse.com/bci/golang:1.19
commands:
- zypper -n install tar gzip
- "curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s v1.49.0;"
Copy link
Contributor

@rmweir rmweir Feb 16, 2023

Choose a reason for hiding this comment

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

Is it necessary to do this for ARM as well? It's redundant afaik; I believe rancher only does it for AMD. If there's no benefit then remove. CI script has check for binary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

@rmweir rmweir left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for doing this 😄.

@mattfarina mattfarina merged commit d73a46f into rancher:master Feb 21, 2023
@mattfarina mattfarina added the needs-pick Needs to be cherry picked to release branches label Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-pick Needs to be cherry picked to release branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants