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.

Sign up for GitHub

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

Jump to bottom

chore: remove some commented code and simplify some #983

Merged
merged 1 commit into from
May 27, 2022
Merged

chore: remove some commented code and simplify some #983

merged 1 commit into from
May 27, 2022

Conversation

hezhizhen
Copy link
Contributor

Ⅰ. Describe what this PR does

  1. remove commented code
  2. tweak imports order (automatically done by GoLand)
  3. use existing functions to replace duplicated parts

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

@kruise-bot kruise-bot requested review from Fei-Guo and furykerry May 26, 2022 08:28
@kruise-bot kruise-bot added the size/L size/L: 100-499 label May 26, 2022
FillZpp
FillZpp reviewed May 26, 2022
View reviewed changes
Copy link
Member

@FillZpp FillZpp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for contributing! One nit:

pkg/webhook/util/configuration/configuration.go
}
if host := webhookutil.GetHost(); len(host) > 0 && wh.ClientConfig.Service != nil {
Copy link
Member

Choose a reason for hiding this comment

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

You may also have to optimize the same code in line 102-104

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

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 it and PTAL.

@FillZpp
Copy link
Member

FillZpp commented May 26, 2022 •
edited
Loading

It seems UT failed because of go.mod changed after make command. Please make generate and then push again.

@FillZpp
Copy link
Member

FillZpp commented May 27, 2022

@hezhizhen Hey, could you fix this today, for we are preparing for v1.2 release and I hope this can be ready for it.

@hezhizhen
Copy link
Contributor Author

@hezhizhen Hey, could you fix this today, for we are preparing for v1.2 release and I hope this can be ready for it.

I tried yesterday, but nothing's different. I guess that it is because my go version is 1.18. I'll fix it today.

@FillZpp
Copy link
Member

FillZpp commented May 27, 2022

I tried yesterday, but nothing's different. I guess that it is because my go version is 1.18. I'll fix it today.

Oh it might be. We have seen go versions always make these problems. Can go mod tidy do any help?

@hezhizhen
chore: remove some commented code and simplify some
13e2cee 
Signed-off-by: Zhizhen He <hezhizhen.yi@gmail.com>
@hezhizhen
Copy link
Contributor Author

I tried yesterday, but nothing's different. I guess that it is because my go version is 1.18. I'll fix it today.

Oh it might be. We have seen go versions always make these problems. Can go mod tidy do any help?

Screen Shot 2022-05-27 at 11 49 50

It seems that go 1.17 and 1.18 handle dependencies differently. 1.17 treats github.com/go-openapi/spec as indirect and 1.18 treats it as direct.

Is there a way to fix go version for this project to avoid cases like this?

@FillZpp
Copy link
Member

FillZpp commented May 27, 2022

Is there a way to fix go version for this project to avoid cases like this?

Emm... for this case, putting _ "github.com/go-openapi/spec" into hack/tools.go explicitly might solve the problem. Not sure if there can be some better ways.

@hezhizhen
Copy link
Contributor Author

Is there a way to fix go version for this project to avoid cases like this?

Emm... for this case, putting _ "github.com/go-openapi/spec" into hack/tools.go explicitly might solve the problem. Not sure if there can be some better ways.

For this case, yes; for general problems caused by different go versions, no.

FillZpp
FillZpp approved these changes May 27, 2022
View reviewed changes
Copy link
Member

@FillZpp FillZpp left a comment

Choose a reason for hiding this comment

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

/lgtm

@kruise-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FillZpp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kruise-bot kruise-bot merged commit 0c6b339 into openkruise:master May 27, 2022
diannaowa pushed a commit to diannaowa/kruise that referenced this pull request Sep 14, 2022
@hezhizhen @diannaowa
chore: remove some commented code and simplify some (openkruise#983)
223b903 
Signed-off-by: Zhizhen He <hezhizhen.yi@gmail.com>
Signed-off-by: Liu Zhenwei <zwliu@thoughtworks.com>
ppbits pushed a commit to ppbits/kruise that referenced this pull request Apr 4, 2024
@hezhizhen
chore: remove some commented code and simplify some (openkruise#983)
1c145ae
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FillZpp FillZpp FillZpp approved these changes

@Fei-Guo Fei-Guo Awaiting requested review from Fei-Guo

@furykerry furykerry Awaiting requested review from furykerry

Assignees

@FillZpp FillZpp

Labels
approved lgtm size/L size/L: 100-499
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hezhizhen @FillZpp @kruise-bot