-
Notifications
You must be signed in to change notification settings - Fork 615
build: check reachable git commits #1592
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
Conversation
9504f80 to
cf4f25b
Compare
cf4f25b to
fd0d31a
Compare
Signed-off-by: CrazyMax <[email protected]>
fd0d31a to
3ab04ce
Compare
thaJeztah
left a comment
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.
LGTM, but left some thoughts 😂
| // Override the locale to ensure consistent output | ||
| cmd.Env = append(os.Environ(), "LC_ALL=C") |
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.
Before I facepalm myself for not asking; did someone look (or do we need a tracking ticket) on the BuildKit side if we should also set this option there? (I think BuildKit also shells out to git - it may be in a more predictable environment, but possibly could still be a good thing to do)
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 might be useful on BuildKit too so we have a consistent output when users report issues.
Signed-off-by: CrazyMax <[email protected]>
3ab04ce to
fd58841
Compare
thaJeztah
left a comment
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.
LGTM
fixes #1587
We should check if the working tree has git commits before retrieving the git commit so it would not fail. I was also thinking of just silently fail. Let me know what sounds best.
Signed-off-by: CrazyMax [email protected]