Skip to content

Use git client in extension manager#6547

Merged
samcoe merged 6 commits into
trunkfrom
extension-git-client
Nov 10, 2022
Merged

Use git client in extension manager#6547
samcoe merged 6 commits into
trunkfrom
extension-git-client

Conversation

@samcoe

@samcoe samcoe commented Nov 1, 2022

Copy link
Copy Markdown
Contributor

This PR changes the ExtensionManager to utilize the new git.Client. It required a bit of refactoring, mostly creating a gitClient interface for just the ExtensionManager to use, in order to make everything testable. Overall functionality of ExtensionManager should not change at all.

@samcoe samcoe self-assigned this Nov 1, 2022
Comment thread pkg/cmd/extension/git.go Outdated
return g.client.Clone(context.Background(), cloneURL, cloneArgs, mods...)
}

func (g *gitExecuter) CommandOutput(args []string, mods ...git.CommandModifier) ([]byte, error) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Combine Command() and Output() methods for easier testing.

Comment thread pkg/cmd/extension/git.go
@samcoe samcoe force-pushed the extension-git-client branch from 8c3028b to 0d2a30e Compare November 2, 2022 10:17
Comment on lines -32 to -40
// git init should create the directory named by argument
if len(args) > 2 && strings.HasPrefix(strings.Join(args, " "), "git init") {
dir := args[len(args)-1]
if !strings.HasPrefix(dir, "-") {
if err := os.MkdirAll(dir, 0755); err != nil {
return err
}
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

git init calls are now mocked out so never reach here.

This comment was marked as spam.

@samcoe samcoe force-pushed the extension-git-client branch from 0d2a30e to 797ef46 Compare November 2, 2022 10:35
defer reg.Verify(t)
client := http.Client{Transport: &reg}
m := newTestManager(tempDir, &client, ios)
gc := &gitExecuter{client: &git.Client{}}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This test uses run.Stub() which git.Client abides by, so there was no need to use the mockGitClient here.

This comment was marked as spam.


func TestManager_Create(t *testing.T) {
chdirTemp(t)
err := os.MkdirAll("gh-test", 0755)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Manually create the extension directory now, but we have assertions that git init is executed which will do this folder creation in production.

@samcoe samcoe marked this pull request as ready for review November 2, 2022 10:46
@samcoe samcoe requested a review from a team as a code owner November 2, 2022 10:46
@samcoe samcoe requested review from mislav and removed request for a team November 2, 2022 10:46
Comment thread git/command.go
Base automatically changed from git-client-cleanup-2 to trunk November 3, 2022 10:58
Comment thread pkg/cmd/extension/manager.go Outdated

localSha, err := cmd.Output()
dir := filepath.Join(m.installDir(), extension)
localSha, err := m.gitClient.CommandOutput([]string{"rev-parse", "HEAD"}, git.WithRepoDir(dir))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wild idea: instead of passing repo dir to each individual invocation, and thus having to modify underlying GitClient methods to accept ...mods as argument, how about adding the ability to create an instance of GitClient for a specific repo? Something like:

scopedClient := gitClient.ForRepo(dir)
scopedClient.Config("remote.origin.url")
scopedClient.CheckoutBranch("branch")

@samcoe samcoe Nov 7, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mislav While I think that would work, it seems more difficult to test.

func (m *Manager) getCurrentVersion(extension string) string {
	dir := filepath.Join(m.installDir(), extension)
        scopedClient := m.gitClient.ForRepo(dir)
	localSha, err := scopedClient.CommandOutput([]string{"rev-parse", "HEAD"})
	if err != nil {
		return ""
	}
	return string(bytes.TrimSpace(localSha))
}
 
func TestGetCurrentVersion(t *testing.T) {
	tempDir := t.TempDir()
	dir := filepath.Join(tempDir, "extensions", "gh-hello")
	gc1 := &mockGitClient{}
	gc2 := &mockGitClient{}
        // We would require two mock git clients...
	gc1.On("ForRepo", dir).Return(gc2).Once()
	gc2.On("CommandOutput", []string{"rev-parse", "HEAD"}).Return("", nil).Once()
	m := newTestManager(tempDir, nil, gc1, nil)
	version := m.getCurrentVersion("gh-hello")
	assert.Equal(t, 1, version)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and implemented this, let me know what you think of the changes.

@samcoe samcoe force-pushed the extension-git-client branch from cc7d27e to 6eff8a9 Compare November 3, 2022 11:12
@samcoe samcoe force-pushed the extension-git-client branch from b7a73b2 to 347180e Compare November 3, 2022 13:45
@samcoe samcoe force-pushed the extension-git-client branch from 347180e to 0dac256 Compare November 3, 2022 14:15
Comment thread pkg/cmd/extension/manager.go Outdated
@samcoe samcoe merged commit 6bbfc50 into trunk Nov 10, 2022
@samcoe samcoe deleted the extension-git-client branch November 10, 2022 09:38
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