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

fix: goimports failed in make all #583

Merged
merged 6 commits into from
May 20, 2022

Conversation

rayowang
Copy link
Member

What this PR does:
I also encountered a similar problem before. After I cloned the registry from github, it was ok to map it to the registry corresponding to golang.org. But a better solution might be to use a good VPN.
Which issue(s) this PR fixes:
#569

@mosn-community-bot mosn-community-bot bot added bug Something isn't working cla:yes size/S labels May 19, 2022
@rayowang rayowang changed the title fix: make all goimports failed fix: goimports failed in make all May 19, 2022
@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #583 (bb91db8) into main (e1a0b55) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #583   +/-   ##
=======================================
  Coverage   60.70%   60.70%           
=======================================
  Files         120      120           
  Lines        6382     6382           
=======================================
  Hits         3874     3874           
  Misses       2139     2139           
  Partials      369      369           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1a0b55...bb91db8. Read the comment docs.

@seeflood seeflood requested a review from Xunzhuo May 19, 2022 12:12
make/golang.mk Outdated Show resolved Hide resolved
Copy link
Member

@Xunzhuo Xunzhuo left a comment

Choose a reason for hiding this comment

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

Thanks @rayowang ,but actually this is not what I expect.

In the new commands, we will clone three repos which are not in small size too and it contains some tools we do not need, and we need to link them(Isn`t it too complex?).

Why do not we just use go get goimports to just install goimports? We will also face the network issue again even if we choose to clone it from github. Dealing with the network issue should not be a task from the team in my view.

@seeflood
Copy link
Member

seeflood commented May 19, 2022

Why do not we just use go get goimports to just install goimports? We will also face the network issue again even if we choose to clone it from github.

@Xunzhuo Because golang.org is 100% banned, but github.com is "sometimes" banned. So chinese users can "sometimes" download goimports from github succesfully, but can never download from golang.org
.......... 😢

How about we use docker to run goimports? It can solve the network issue (by using a dockerhub mirror site) and preventing errors caused by different versions of goimports

Or.....we add some new commands, e.g. make format.docker , make all.docker and keep the existing commands clean

@rayowang rayowang closed this May 19, 2022
@rayowang rayowang reopened this May 19, 2022
@rayowang
Copy link
Member Author

rayowang commented May 19, 2022

Thanks @rayowang ,but actually this is not what I expect.

In the new commands, we will clone three repos which are not in small size too and it contains some tools we do not need, and we need to link them(Isn`t it too complex?).

Why do not we just use go get goimports to just install goimports? We will also face the network issue again even if we choose to clone it from github. Dealing with the network issue should not be a task from the team in my view.

@Xunzhuo I just saw the message, I think what you said makes sense, but as seeflood @seeflood said, it is impossible to access golang.org in China. In fact, some VPN tools may not work. The person who opened the issue before has been doing it for a long time. Of course, it is better to build in docker, but this will lose the convenience of local build, so this way is a temporary compromise.

@rayowang rayowang requested review from seeflood and Xunzhuo May 20, 2022 02:30
@seeflood
Copy link
Member

@Xunzhuo Any idea on this PR?
I'm ok with it.
An alternative is we first ping golang.org . If the ping returns successfully, we can download the goimports from golang.org directly.
Anyway, both solutions are ok for me.

@Xunzhuo
Copy link
Member

Xunzhuo commented May 20, 2022

I am Okay with this, thanks @rayowang

Copy link
Member

@seeflood seeflood 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!

@seeflood seeflood merged commit 3565211 into mosn:main May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla:yes size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants