[go: up one dir, main page]
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

clean package "io/ioutil" ,because "io" and "os" can replaced it totally #22016

Merged
merged 1 commit into from
Nov 8, 2022
Merged

clean package "io/ioutil" ,because "io" and "os" can replaced it totally #22016

merged 1 commit into from
Nov 8, 2022

Conversation

yanggangtony
Copy link
Contributor

Signed-off-by: yanggang gang.yang@daocloud.io

We should clean all io/util package according the url: https://go.dev/doc/go1.16#ioutil
/kind cleanup

@yanggangtony yanggangtony requested review from a team as code owners November 6, 2022 10:03
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 6, 2022
pchaigno
pchaigno requested changes Nov 6, 2022
View reviewed changes
Copy link
Member
@pchaigno pchaigno left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your pull request!

Could you explain, in the commit description, why you are making this change?

Also, what does "for advanced Golang" mean? Did you mean "to accommodate newer Golang versions"?

@pchaigno pchaigno added the release-note/misc This PR makes changes that have no direct user impact. label Nov 6, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 6, 2022
@yanggangtony
Copy link
Contributor Author

Thanks for your pull request!

Could you explain, in the commit description, why you are making this change?

Also, what does "for advanced Golang" mean? Did you mean "to accommodate newer Golang versions"?

@pchaigno
I mean the io/ioutil package is obsolete , maybe will be removed in newer version of golang.
Now it is replaced well by io or os package..

@pchaigno
Copy link
Member
pchaigno commented Nov 7, 2022

Thanks for your pull request!
Could you explain, in the commit description, why you are making this change?
Also, what does "for advanced Golang" mean? Did you mean "to accommodate newer Golang versions"?

@pchaigno I mean the io/ioutil package is obsolete , maybe will be removed in newer version of golang. Now it is replaced well by io or os package..

I understand that but we need that explanation in the commit message.

@yanggangtony
Copy link
Contributor Author

I understand that but we need that explanation in the commit message.
@pchaigno
Hi , i have updated the commit message ..

@yanggangtony yanggangtony changed the title remove io/ioutil for advanced golang clean package "io/ioutil" , because "io" and "os" can replaced it totally Nov 7, 2022
@pchaigno
Copy link
Member
pchaigno commented Nov 7, 2022

The commit is ill-formatted. There should be two new lines between the commit title and it's description and the commit title should be less than 75 characters. This is why GitHub and checkpatch are currently confused.

@yanggangtony
Copy link
Contributor Author

The commit is ill-formatted. There should be two new lines between the commit title and it's description and the commit title should be less than 75 characters. This is why GitHub and checkpatch are currently confused.

@pchaigno
thanks ,that's better now..

@pchaigno
Copy link
Member
pchaigno commented Nov 7, 2022

It's better but your title is still:

clean package "io/ioutil" because "io" and "os" package can replaced it totally. according to the url : https://go.dev/doc/go1.16#ioutil

You need two newlines to separate a commit's title from its description.

clean package "io/ioutil"
c86f23c 
because "io" and "os" package can replaced it totally.
according to the url :
https://go.dev/doc/go1.16#ioutil

Signed-off-by: yanggang <gang.yang@daocloud.io>
@yanggangtony yanggangtony changed the title clean package "io/ioutil" , because "io" and "os" can replaced it totally clean package "io/ioutil" ,because "io" and "os" can replaced it totally Nov 7, 2022
@yanggangtony
Copy link
Contributor Author

@pchaigno

You need two newlines to separate a commit's title from its description.

now , the commit style is ok?
i add two newlines..:smile::smile:

@pchaigno
Copy link
Member
pchaigno commented Nov 8, 2022

@yanggangtony Yep, looks good now. Next time I'd advise to look at it with git log and you'll see that the previous version broke the formatting.

@yanggangtony
Copy link
Contributor Author

@yanggangtony Yep, looks good now. Next time I'd advise to look at it with git log and you'll see that the previous version broke the formatting.

@pchaigno
thanks...

@pchaigno
Copy link
Member
pchaigno commented Nov 8, 2022

The smoke tests are passing and, given the changes, I don't think we need to run the full end-to-end tests. cilium/proxy is missing a review but the changes for that codeowners aren't really specific to the proxy, so probably okay to skip. Merging.

Thanks @yanggangtony for your contribution!

@pchaigno pchaigno merged commit 181b030 into cilium:master Nov 8, 2022
@pchaigno pchaigno added the kind/cleanup This includes no functional changes. label Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanm aanm aanm approved these changes

@pchaigno pchaigno pchaigno approved these changes

Assignees
No one assigned
Labels
kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yanggangtony @pchaigno @aanm