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

Use .gitmodules for determining what hosts to keyscan #876

Merged
merged 6 commits into from
Dec 18, 2018

Conversation

lox
Copy link
Contributor

@lox lox commented Dec 17, 2018

Currently we use a git submodule foreach to try and get the url of all our submodules so that we can figure out if any need ssh keyscanning. This is unfortunately completely flawed as none of them exist until after we call git submodule update.

This uses the git config command to parse the .gitmodule file instead.

Closes #825.

lox added 2 commits December 17, 2018 11:52
Currently we use a git submodule foreach to try and get the url of all
our submodules so that we can figure out if any need ssh keyscanning.
This is unfortunately completely flawed as none of them exist until
after we call git submodule update.

This uses the git config command to parse the .gitmodule file instead.
@lox lox force-pushed the use-gitmodules-for-keyscan branch from c2c4d53 to 495969c Compare December 17, 2018 01:25
@lox lox requested a review from a team December 17, 2018 23:43
@keithpitt
Copy link
Member

Nice! (out of interest, do those calls change our minimum git version by any chance?)

@keithpitt
Copy link
Member

(i.e. get-regexp sounds neat, and I’m wondering if it’s a later feature or something)

@lox
Copy link
Contributor Author

lox commented Dec 18, 2018

What actually is our minimum git version?

@lox
Copy link
Contributor Author

lox commented Dec 18, 2018

Looks like it's been in there for ages, I can see it in 1.7

@matthewd
Copy link
Contributor

Do we need to consider nested submodules? Looks like the previous command attempted to handle them with --recursive (also illustrated in the previous example output)

I wonder whether we could we feature-detect whether ssh is new enough to support -o StrictHostKeyChecking=accept-new.

@lox
Copy link
Contributor Author

lox commented Dec 18, 2018

Neat, I haven't seen -o StrictHostKeyChecking=accept-new @matthewd

@lox
Copy link
Contributor Author

lox commented Dec 18, 2018

Given the current implementation just flat out doesn't work, I'd be inclined to go with this vs trying to solve recursive @matthewd.

Perhaps we can add StrictHostKeyChecking=accept-new as an experiment with the aim of making it the default at some point in the future.

bootstrap/git.go Outdated
output, err := sh.RunAndCapture(
"git", "submodule", "foreach", "--recursive", "git", "ls-remote", "--get-url")
"git", "config", "--file", ".gitmodules", "--null", "--get-regexp", "url")
Copy link
Member

Choose a reason for hiding this comment

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

Neat! url seems like a very broad regexp, maybe ^submodule\..+\.url$?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

⚡️

// every third element to get the [email protected]:blah bit
if idx%3 == 2 {
urls = append(urls, val)
// splits lines on null-bytes to gracefully handle line endings and repositories with newlines
Copy link
Member

Choose a reason for hiding this comment

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

repositories with newlines

is this possible?? wow, git

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think using null-byte delimiters ends up being better practice anyway.

bootstrap/git.go Outdated
for _, line := range lines {
tokens := strings.SplitN(line, "\n", 2)
if len(tokens) != 2 {
return nil, fmt.Errorf("Failed to parse .gitmodule line %q", line)
Copy link
Member

Choose a reason for hiding this comment

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

.gitmodules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

⚡️

Copy link
Member

@sj26 sj26 left a comment

Choose a reason for hiding this comment

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

Nice one, @lox! It really wasn't working before, huh? I guess most folks would use the same host for their main repo and submodules, probably github.com, so we haven't noticed before?

@lox
Copy link
Contributor Author

lox commented Dec 18, 2018

Yeah, totally non-functional!

@lox
Copy link
Contributor Author

lox commented Dec 18, 2018

Trying to integration test the submodules stuff is a bit awkward, so we missed it entirely.

@keithpitt
Copy link
Member

-o StrictHostKeyChecking=accept-new changes everything! (that's very mewl)

@keithpitt
Copy link
Member

(kewl even)

@lox lox merged commit 2ae66c6 into master Dec 18, 2018
@lox lox deleted the use-gitmodules-for-keyscan branch December 18, 2018 01:05
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.

4 participants