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

clean the unused code and prune vineyardctl dependency #1562

Merged
merged 7 commits into from
Sep 18, 2023

Conversation

zhuyi1159
Copy link
Contributor

What do these changes do?

as titled

Related issue number

Fixes #1268

@netlify
Copy link

netlify bot commented Sep 11, 2023

Deploy Preview for v6d ready!

Name Link
🔨 Latest commit be7cfe6
🔍 Latest deploy log https://app.netlify.com/sites/v6d/deploys/6502ca7e559ee30008f99a46
😎 Deploy Preview https://deploy-preview-1562--v6d.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@sighingnow sighingnow left a comment

Choose a reason for hiding this comment

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

Hi @zhuyi1159 Using testify in _test.go modules should be fine.

@zhuyi1159
Copy link
Contributor Author

But testify is not widely used. Replacing testifywith the Golang built-in package reflect can reduce two dependencies (there is also an indirect dependency for testify )

@dashanji
Copy link
Member

@zhuyi1159 Agree to reserve the testify, could you please update the following code from the ginkgo framework to the testify framework? Only reserving one test framework is fine. Also, please update the relevant file name as the test file will be rewritten while adding a new CRD.

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

@dashanji
Copy link
Member

@zhuyi1159 Have you tested the webhook_test.go and controller_test.go locally?

@zhuyi1159
Copy link
Contributor Author

Not tested locally.

}

var _ = BeforeSuite(func() {
func Test_BeforeSuite(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

I think you'd better to under how the ginkgo framework works. BeforeSuite is used to build the environment for the test suit, you can't update it as a single test function. Also, please check the other similar parts.


var _ = AfterSuite(func() {
func Test_AfterSuite(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Same as before.

@dashanji
Copy link
Member

Not tested locally.

Could you please add the relevant unit test on CI or test locally? Thank you.

@dashanji
Copy link
Member

@zhuyi1159 Please update

Paths: []string{filepath.Join("..", "..", "..", "config", "webhook")},

to

Paths: []string{filepath.Join("..", "..", "..", "config", "webhook","manifests.yaml")}, 

@dashanji
Copy link
Member

Also, please make sure the version of kube-apiserver > v1.19.2

Signed-off-by: zhuyi1159 <1159751291@qq.com>
Signed-off-by: zhuyi1159 <1159751291@qq.com>
Signed-off-by: zhuyi1159 <1159751291@qq.com>
Signed-off-by: zhuyi1159 <1159751291@qq.com>
Signed-off-by: zhuyi1159 <1159751291@qq.com>
@zhuyi1159
Copy link
Contributor Author

Local tests passed.

@dashanji dashanji marked this pull request as ready for review September 15, 2023 05:53
Copy link
Member

@dashanji dashanji left a comment

Choose a reason for hiding this comment

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

Basically LGTM, just one nit.

k8s/apis/k8s/v1alpha1/webhook_test.go Outdated Show resolved Hide resolved
Signed-off-by: zhuyi1159 <1159751291@qq.com>
@sighingnow sighingnow merged commit 172d18b into v6d-io:main Sep 18, 2023
23 checks passed
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.

Reduce/optimize the binary size of current vineyardctl
3 participants