-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
dev-doctor: add check for the root directory #16205
Conversation
return checkError, fmt.Sprintf("cannot get working directory: %s", err) | ||
} | ||
|
||
// Search upward through through parent directories to find the .git directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not immediately clear to me why we search upward. Is doctor run from any subdir in cilium rather than root, ro what's the main reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although running make dev-doctor
will run from the root, folks might also run cd tools/dev-doctor && go run .
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about empty GOPATH, or maybe we don't even want to bother.
tools/dev-doctor/rootdircheck.go
Outdated
info, err := os.Stat(path.Join(dir, ".git")) | ||
switch { | ||
case err == nil && info.Mode().IsDir(): | ||
if dir != path.Join(os.Getenv("GOPATH"), "src", "github.com", "cilium", "cilium") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if GOPATH==""
this could succeed even though in reality wouldn't work, right? Perhaps we could check for !!GOPATH
first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, done.
tools/dev-doctor/rootdircheck.go
Outdated
|
||
// A rootDirCheck checks that Cilium is checked out in | ||
// ${GOPATH}/src/github.com/cilium/cilium. | ||
type rootDirCheck struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you extract ${GOPATH}/src/github.com/cilium/cilium
to a default variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, done.
tools/dev-doctor/rootdircheck.go
Outdated
info, err := os.Stat(path.Join(dir, ".git")) | ||
switch { | ||
case err == nil && info.Mode().IsDir(): | ||
if dir != path.Join(os.Getenv("GOPATH"), "src", "github.com", "cilium", "cilium") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check above comment at L27. This is can then be os.State(rootDirCheck.path)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, lgtm!
Currently Cilium's build infrastructure assumes that Cilium is checked out in ${GOPATH}/src/github.com/cilium/cilium, and will fail in unpredictable ways if it is not. This PR adds a check to the dev-doctor target to alert developers when this is the case. Signed-off-by: Tom Payne <tom@isovalent.com>
Currently Cilium's build infrastructure assumes that Cilium is checked
out in ${GOPATH}/src/github.com/cilium/cilium, and will fail in
unpredictable ways if it is not.
This PR adds a check to the dev-doctor target to alert developers when
this is the case.
Signed-off-by: Tom Payne tom@isovalent.com
Refs #11341