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

feat(git): log parsed gitURL and warn if local #345

Merged
merged 4 commits into from
Sep 12, 2024
Merged

Conversation

johnstcn
Copy link
Member

@johnstcn johnstcn commented Sep 12, 2024

Part of #281

If we go about fetching changes from a remote repo, we're going to want to log some more information about what's going on.
I also took this opportunity to log some more information about the git URL we parse.
Specifically, if a user passes a git URL in the form of OrgName/RepoName our library for parsing git URLs will interpret that as a local URL (file://).
go-git will fall back to the git executable in this case, which is likely to fail if git is not present. I'm choosing to log a warn but not block in case someone decides to make their own envbuilder image with git installed.

@johnstcn johnstcn self-assigned this Sep 12, 2024
git/git.go Outdated Show resolved Hide resolved
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

Nice, thanks for fixing a bunch of old inconsistencies!

envbuilder.go Outdated Show resolved Hide resolved
@@ -47,11 +46,12 @@ type CloneRepoOptions struct {
// be cloned again.
//
// The bool returned states whether the repository was cloned or not.
func CloneRepo(ctx context.Context, opts CloneRepoOptions) (bool, error) {
func CloneRepo(ctx context.Context, logf func(string, ...any), opts CloneRepoOptions) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be optional as part of options, but not a blocker.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it is optional! If it's not provided, we'll try to log and panic.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, it would be via != nil check or assigning a noop function.

return func(hostname string, remote net.Addr, key gossh.PublicKey) error {
var sb strings.Builder
_ = knownhosts.WriteKnownHost(&sb, hostname, remote, key)
// skeema/knownhosts uses a fake public key to determine the host key
// algorithms. Ignore this one.
if s := sb.String(); !strings.Contains(s, "fake-public-key ZmFrZSBwdWJsaWMga2V5") {
logger(log.LevelInfo, "#1: 🔑 Got host key: %s", strings.TrimSpace(s))
logger("🔑 Got host key: %s", strings.TrimSpace(s))
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@johnstcn johnstcn merged commit e14b952 into main Sep 12, 2024
4 checks passed
@johnstcn johnstcn deleted the cj/log-git-url branch September 12, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants