[supervisor] inflate all git repos

[svenefftinge]

fixes [multi-repo] Shallow clones are not inflated #9021

How to test

start a workspace on a multi-repo. E.g. https://sefftinge-362778b072.preview.gitpod-dev.com/#https://github.com/svenefftinge/multi-repo-main
And verify that the environment variable GITPOD_REPO_ROOTS contains all three locations (comma separated) and all repositories are fully cloned (check with git log).

NONE

[akosyakov]

lgtm and works as advertised

[csweichel]

Change makes a ton of sense.

Got a few comments.
Also, I didn't see a CDWP fixture which actually has multiple roots. Is that because we don't have a CDWP fixture with a composite initializer, or is there something that's broken?

[csweichel]

Wouldn't this function replace GetCheckoutLocationFromInitializer as in

var (
    repoRoot string
    roots    = GetCheckoutLocationsFromInitializer(init)
)
if len(roots) > 0 {
    repoRoot = roots[0]
}

?

If so, I'd vote for removing GetCheckoutLocationFromInitializer in favour of this new function.

[svenefftinge]

The single (main) repo_root is unfortunately used in a few other places, so I opted at adding a second variant. So we can slowly migrate based on need.

[svenefftinge]

Ah, I understand now. 👍

[csweichel]

should return an empty list instead

Suggested change

	return []string{""}
	return nil

[csweichel]

Suggested change

	for _, path := range GetCheckoutLocationsFromInitializer(c) {
	result = append(result, path)
	}
	result = append(result, GetCheckoutLocationsFromInitializer(c)...)

[csweichel]

this should be a table driven test:

import "github.com/google/go-cmp/cmp"

type Expectation struct {
    Roots []string
}
tests := []struct{
    Name string
    Initializer *csapi.WorkspaceInitializer
}{
    {
        Name: "single git initializer",
        Initializer: ...
    }
}

for _, test := range tests {
    t.Run(test.Name, func(t *testing.T) {
        var act Expectation
        act.Roots = initializer.GetCheckoutLocationsFromInitializer(test.Initializer)

        if diff := cmp.Diff(test.Expectation, act); diff != "" {
            t.Errorf("unexpected result (-want +got):\n%s", diff)
        }
    })
}

[csweichel]

nit: TODO comments

Suggested change

	//TODO remove me after deployment (backward compatibility)
	//TODO(sefftinge) remove me after deployment (backward compatibility)

[svenefftinge]

Also, I didn't see a CDWP fixture which actually has multiple roots. Is that because we don't have a CDWP fixture with a composite initializer, or is there something that's broken?

Yes, the reason is I couldn'T find a git initializer but only snapshot initializers. Maybe I missed something?