feat: Add ability to configure repository clone directory #366

filipkrayem · 2023-07-03T14:50:39Z

What does this change

Adds the ability to configure repository clone directory.

What issue does it fix

Closes #294

Notes for the reviewer

Hi! I'm a newcomer to Go, so the code probably isn't really idiomatic, any comments and reviews are more than welcome!

Checklist

Made sure the PR follows the CONTRIBUTING.md guidelines
Tests if something new is added

lindell

Thanks for the PR!

I'm impressed with the code as a Go beginner 🥇 But there are some things that need to be changed.

Feel free to reach out if anything is unclear.

tests/repo_helper_test.go

tests/table_test.go

internal/multigitter/shared.go

tests/table_test.go

internal/multigitter/shared.go

filipkrayem · 2023-07-13T08:10:32Z

Thanks for the thorough review! I implemented what you asked for, however there's one more thing I am not so sure about and that is the test. Couldn't really figure out what exactly to test for apart from a folder existing in the directory. Do you have any ideas on how to improve the test?

lindell

Code look good except two minor things left behind from the code comment refactoring.

When it comes to the test, it's a bit tricky, since multi-gitter should always delete the directory where things are cloned to after the execution is complete. So we will have to, from the script that is run, leave something behind.

I can think of two approaches.

Create a program/script that prints the current location, then check it in the logs

// tests/scripts/pwd/main.go
package main

import (
	"fmt"
	"os"
)

func main() {
	path, _ := os.Getwd()
	fmt.Println("Current path:", path)
}

A similar approach, expect printing the current location is saved to a file (which is then pushed to the "remote" by multi-gitter). Then we can verify what was committed and ensure that a specific file contains the text of the directory we expect.

Nr. 1 is probably the easiest.

internal/multigitter/shared.go

filipkrayem · 2023-07-19T20:53:20Z

Ended up adding it to the logs since creating a script would require changing the cobra.Command.Args config and that probably wouldn't be the best solution.

lindell

Ended up adding it to the logs since creating a script would require changing the cobra.Command.Args config and that probably wouldn't be the best solution.

I don't really know what is meant by cobra.Command.Args having to be changed. But just checking the logs of where it claims the repo will be cloned into will not actually test that this feature does work.

I implemented the test as suggested (, and added an absolute path test), based on the commit of the last review, and it seems to work without any cobra changes. e5f0c90

lindell · 2023-07-22T07:40:17Z

internal/multigitter/print.go

@@ -67,7 +67,9 @@ func (r Printer) runSingleRepo(ctx context.Context, repo scm.Repository) error {
 	log := log.WithField("repo", repo.FullName())
 	log.Info("Cloning and running script")

-	tmpDir, err := CreateTempDir(r.CloneDir)
+	tmpDir, err := createTempDir(r.CloneDir)
+	log.Info("Cloning into directory ", NormalizePath(tmpDir))


A log message is good. But it should be max debug level.

Why is the path normalized for the logging?

Because the path wouldn't match on windows devices when not normalized

lindell · 2023-07-22T10:31:20Z

tests/table_test.go

@@ -1001,6 +1002,7 @@ Repositories with a successful run:
 			verify: func(t *testing.T, vcMock *vcmock.VersionController, runData runData) {
 				require.Len(t, vcMock.PullRequests, 1)
 				assert.True(t, fileExist(t, workingDir, "tmp-test"))
+				assert.Contains(t, runData.logOut, multigitter.NormalizePath(filepath.Join(workingDir, "tmp-test/")))


Just checking where the logs claim the repo is cloned to will not guarantee that it will actually use that directory.

tmpDir, err := createTempDir(r.CloneDir) log.Debug("Cloning into directory ", NormalizePath(tmpDir)) CloneAndRun(os.TempDir())

⬆️ Pseudocode that would pass the test.

lindell · 2023-07-22T10:41:13Z

internal/multigitter/shared.go

+	}
+
+	// Create the directory
+	err := os.MkdirAll(directoryPath, os.ModePerm)


0777 is higher permissions than needed for this directory. Just read and write access for the current user should be enough.

filipkrayem · 2023-08-01T07:41:37Z

The tests work locally, even when run inside the github actions with act, so the issue is, it seems, related to the github environment where the actions are run. Any ideas?

lindell · 2023-08-09T18:14:49Z

It might be that 0600 is not enough for the directory creation. Try to add execution permission as well (0700).

filipkrayem · 2024-05-01T12:37:38Z

Hi, sorry, this PR got kind of forgotten 😄 I added the 0700 perms and resolved the merge conflict 👍

lindell

Thanks for the contribution 😄

github-actions · 2024-05-04T06:26:54Z

Included in release v0.52.0 🎉

feat: Add ability to configure repository clone directory

596a21b

lindell requested changes Jul 4, 2023

View reviewed changes

code review changes

cd23524

lindell reviewed Jul 13, 2023

View reviewed changes

internal/multigitter/shared.go Outdated Show resolved Hide resolved

internal/multigitter/shared.go Outdated Show resolved Hide resolved

lindell force-pushed the master branch 2 times, most recently from de07efa to 2e9b182 Compare July 15, 2023 19:12

add clone dir to logs and check it in test

1d651dc

normalize path when logging clone dir location

ded3aa7

lindell reviewed Jul 22, 2023

View reviewed changes

filipkrayem added 2 commits July 24, 2023 18:59

add tests from @lindell

1b6f96e

commit to re-run actions

a87f48c

filipkrayem and others added 2 commits August 12, 2023 10:52

make directory with more permissions

b94d727

Merge branch 'master' into clone-dir

3d77111

filipkrayem requested a review from lindell May 3, 2024 07:27

lindell added 2 commits May 4, 2024 08:12

Merge branch 'master' into clone-dir

b881870

gofmt

74acd9f

lindell approved these changes May 4, 2024

View reviewed changes

lindell merged commit 6345c6b into lindell:master May 4, 2024
9 checks passed

github-actions bot mentioned this pull request May 4, 2024

chore(master): release 0.52.0 #470

Merged

github-actions bot mentioned this pull request Jul 2, 2024

chore(master): release 0.52.1 #491

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add ability to configure repository clone directory #366

feat: Add ability to configure repository clone directory #366

filipkrayem commented Jul 3, 2023

lindell left a comment

filipkrayem commented Jul 13, 2023

lindell left a comment

filipkrayem commented Jul 19, 2023

lindell left a comment

lindell Jul 22, 2023

lindell Jul 22, 2023

filipkrayem Jul 24, 2023 •

edited

Loading

lindell Jul 22, 2023

lindell Jul 22, 2023

filipkrayem commented Aug 1, 2023

lindell commented Aug 9, 2023

filipkrayem commented May 1, 2024

lindell left a comment

github-actions bot commented May 4, 2024

feat: Add ability to configure repository clone directory #366

feat: Add ability to configure repository clone directory #366

Conversation

filipkrayem commented Jul 3, 2023

What does this change

What issue does it fix

Notes for the reviewer

Checklist

lindell left a comment

Choose a reason for hiding this comment

filipkrayem commented Jul 13, 2023

lindell left a comment

Choose a reason for hiding this comment

filipkrayem commented Jul 19, 2023

lindell left a comment

Choose a reason for hiding this comment

lindell Jul 22, 2023

Choose a reason for hiding this comment

lindell Jul 22, 2023

Choose a reason for hiding this comment

filipkrayem Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

lindell Jul 22, 2023

Choose a reason for hiding this comment

lindell Jul 22, 2023

Choose a reason for hiding this comment

filipkrayem commented Aug 1, 2023

lindell commented Aug 9, 2023

filipkrayem commented May 1, 2024

lindell left a comment

Choose a reason for hiding this comment

github-actions bot commented May 4, 2024

filipkrayem Jul 24, 2023 •

edited

Loading