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

[BUG] make reviewable fails #6457

Closed
shanshanying opened this issue Jan 15, 2024 · 8 comments · Fixed by #6959
Closed

[BUG] make reviewable fails #6457

shanshanying opened this issue Jan 15, 2024 · 8 comments · Fixed by #6959
Assignees
Labels
bug good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/bug Something isn't working Stale
Milestone

Comments

@shanshanying
Copy link
Contributor

According to the development guid
image

One should run make reviewable and make sure it success.

But on the latest main branch, it fails:
image

@shanshanying shanshanying added kind/bug Something isn't working good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. labels Jan 15, 2024
@infdahai
Copy link

It just seems to have an error with the type check.

Copy link

This issue has been marked as stale because it has been open for 30 days with no activity

@github-actions github-actions bot added the Stale label Feb 19, 2024
@Jaxx4Fun
Copy link
Contributor

@shanshanying Hi, I am interested in this issue. I found before the target reviewable is executed, it actually failed in generate stage..

Here is what I see after I run make generate
image

@shanshanying shanshanying assigned infdahai and Jaxx4Fun and unassigned infdahai Mar 25, 2024
@shanshanying
Copy link
Contributor Author

@shanshanying Hi, I am interested in this issue. I found before the target reviewable is executed, it actually failed in generate stage..

Here is what I see after I run make generate image

which branch did you run make generate? could you pls sync the latest main branch and try it out? Thanks.

@Jaxx4Fun
Copy link
Contributor

I have synced and ran make generate just now on the newest main branch and still got the same output...

The commit is bb2f7f6

I put some device info which may help.

go version
go version go1.22.0 darwin/arm64

sw_vers
ProductName:		macOS
ProductVersion:		14.3.1
BuildVersion:		23D60

@shanshanying
Copy link
Contributor Author

shanshanying commented Mar 26, 2024

I have synced and ran make generate just now on the newest main branch and still got the same output...

The commit is bb2f7f6

I put some device info which may help.

go version
go version go1.22.0 darwin/arm64

sw_vers
ProductName:		macOS
ProductVersion:		14.3.1
BuildVersion:		23D60

hi @Jaxx4Fun
what is your controller-gen version?
I am wandering it is a bug of controller-gen, pls check out the issue and find if it is relevant. kubernetes-sigs/controller-tools#888

BTW, this is the version I am working on:

./bin/controller-gen --version
Version: v0.12.1

In KubeBlocks, You can get it by

make controller-gen

@Jaxx4Fun
Copy link
Contributor

I am wandering it is a bug of controller-gen, pls check out the issue and find if it is relevant. kubernetes-sigs/controller-tools#888

Yes. I just update the controller-gen to v0.14.0, which works for me currently. I can continue to work on this issue now..
Thanks! @shanshanying

@Jaxx4Fun
Copy link
Contributor

Jaxx4Fun commented Apr 2, 2024

Append test-go-generate to the dependency list of the target vet

I found the target reviewable depends on target vet, but simply running make vet it says

vet: pkg/configuration/container/container_kill_test.go:101:15: undefined: mocks.NewMockContainerAPIClient
which is generated in target test-go-generate.

Update EXCLUDES_DIRS in hack/license/header-check.sh

I found one of the excluded directory is set to pkg/lorry/component/. But currently there is no directory named component under pkg/lorry. According to the failure cases below, seems to exclude pkg/lorry/engines and pkg/lorry/util is enough, but I am not sure if it is too arbitrary. @shanshanying

Anyway I'd like to make a PR to fix these two issues first..

❯ make check-license-header |grep FAIL
Header check: pkg/lorry/engines/kafka/auth.go... FAIL
Header check: pkg/lorry/engines/kafka/consumer.go... FAIL
Header check: pkg/lorry/engines/kafka/kafka.go... FAIL
Header check: pkg/lorry/engines/kafka/metadata.go... FAIL
Header check: pkg/lorry/engines/kafka/metadata_test.go... FAIL
Header check: pkg/lorry/engines/kafka/producer.go... FAIL
Header check: pkg/lorry/engines/kafka/sarama_log_bridge.go... FAIL
Header check: pkg/lorry/engines/kafka/sasl_oauthbearer.go... FAIL
Header check: pkg/lorry/engines/kafka/scram_client.go... FAIL
Header check: pkg/lorry/engines/kafka/thirdparty/retry.go... FAIL
Header check: pkg/lorry/engines/kafka/utils.go... FAIL
Header check: pkg/lorry/engines/redis/metadata.go... FAIL
Header check: pkg/lorry/engines/redis/redis.go... FAIL
Header check: pkg/lorry/engines/redis/settings.go... FAIL
Header check: pkg/lorry/engines/redis/settings_test.go... FAIL
Header check: pkg/lorry/util/config/decode.go... FAIL
make: *** [check-license-header] Error 1

Jaxx4Fun added a commit to Jaxx4Fun/kubeblocks that referenced this issue Apr 2, 2024
Jaxx4Fun added a commit to Jaxx4Fun/kubeblocks that referenced this issue Apr 2, 2024
The subdirecotries under the path `pkg/lorry/` has been changed, which
requires the header-check script to update the excluded dirs
correspondingly.
@github-actions github-actions bot added this to the Release 0.9.0 milestone Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/bug Something isn't working Stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants