steve/databricks-cli
steve
/
databricks-cli
mirror of https://github.com/databricks/cli.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Enable linter 'copyloopvar' and fix the issues (#2160) 

## Changes
- Remove all unnecessary copies of the loop variable, it is not
necessary since Go 1.22 https://go.dev/blog/loopvar-preview
- Enable the linter that catches this issue
https://github.com/karamaru-alpha/copyloopvar

## Tests
Existing tests.
This commit is contained in:
Denis Bilenko 2025-01-16 12:20:50 +01:00 committed by GitHub
parent a002a24e41
commit b273dc5942
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
11 changed files with 74 additions and 139 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
.golangci.yaml
View File

@ -42,6 +42,8 @@ linters-settings:
disable:
# good check, but we have too many assert.(No)?Errorf? so excluding for now
- require-error
copyloopvar:
check-alias: true
issues:
exclude-dirs-use-default: false # recommended by docs https://golangci-lint.run/usage/false-positives/
max-issues-per-linter: 1000

8
bundle/config/validate/validate_sync_patterns.go
View File

@ -47,15 +47,13 @@ func checkPatterns(patterns []string, path string, rb bundle.ReadOnlyBundle) (di
var errs errgroup.Group
var diags diag.Diagnostics
for i, pattern := range patterns {
index := i
fullPattern := pattern
for index, pattern := range patterns {
// If the pattern is negated, strip the negation prefix
// and check if the pattern matches any files.
// Negation in gitignore syntax means "don't look at this path'
// So if p matches nothing it's useless negation, but if there are matches,
// it means: do not include these files into result set
p := strings.TrimPrefix(fullPattern, "!")
p := strings.TrimPrefix(pattern, "!")
errs.Go(func() error {
fs, err := fileset.NewGlobSet(rb.BundleRoot(), []string{p})
if err != nil {
@ -72,7 +70,7 @@ func checkPatterns(patterns []string, path string, rb bundle.ReadOnlyBundle) (di
mu.Lock()
diags = diags.Append(diag.Diagnostic{
Severity: diag.Warning,
Summary: fmt.Sprintf("Pattern %s does not match any files", fullPattern),
Summary: fmt.Sprintf("Pattern %s does not match any files", pattern),
Locations: []dyn.Location{loc.Location()},
Paths: []dyn.Path{loc.Path()},
})

4
cmd/bundle/generate/utils.go
View File

@ -138,9 +138,7 @@ func (n *downloader) FlushToDisk(ctx context.Context, force bool) error {
}
errs, errCtx := errgroup.WithContext(ctx)
for k, v := range n.files {
targetPath := k
filePath := v
for targetPath, filePath := range n.files {
errs.Go(func() error {
reader, err := n.w.Workspace.Download(errCtx, filePath)
if err != nil {

18
integration/cmd/fs/cat_test.go
View File

@ -18,13 +18,11 @@ func TestFsCat(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
f, tmpDir := tc.setupFiler(t)
f, tmpDir := testCase.setupFiler(t)
err := f.Write(context.Background(), "hello.txt", strings.NewReader("abcd"), filer.CreateParentDirectories)
require.NoError(t, err)
@ -40,13 +38,11 @@ func TestFsCatOnADir(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
f, tmpDir := tc.setupFiler(t)
f, tmpDir := testCase.setupFiler(t)
err := f.Mkdir(context.Background(), "dir1")
require.NoError(t, err)
@ -61,13 +57,11 @@ func TestFsCatOnNonExistentFile(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
_, tmpDir := tc.setupFiler(t)
_, tmpDir := testCase.setupFiler(t)
_, _, err := testcli.RequireErrorRun(t, ctx, "fs", "cat", path.Join(tmpDir, "non-existent-file"))
assert.ErrorIs(t, err, fs.ErrNotExist)

86
integration/cmd/fs/cp_test.go
View File

@ -126,14 +126,12 @@ func TestFsCpDir(t *testing.T) {
t.Parallel()
for _, testCase := range copyTests() {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
sourceFiler, sourceDir := tc.setupSource(t)
targetFiler, targetDir := tc.setupTarget(t)
sourceFiler, sourceDir := testCase.setupSource(t)
targetFiler, targetDir := testCase.setupTarget(t)
setupSourceDir(t, context.Background(), sourceFiler)
testcli.RequireSuccessfulRun(t, ctx, "fs", "cp", sourceDir, targetDir, "--recursive")
@ -147,14 +145,12 @@ func TestFsCpFileToFile(t *testing.T) {
t.Parallel()
for _, testCase := range copyTests() {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
sourceFiler, sourceDir := tc.setupSource(t)
targetFiler, targetDir := tc.setupTarget(t)
sourceFiler, sourceDir := testCase.setupSource(t)
targetFiler, targetDir := testCase.setupTarget(t)
setupSourceFile(t, context.Background(), sourceFiler)
testcli.RequireSuccessfulRun(t, ctx, "fs", "cp", path.Join(sourceDir, "foo.txt"), path.Join(targetDir, "bar.txt"))
@ -168,14 +164,12 @@ func TestFsCpFileToDir(t *testing.T) {
t.Parallel()
for _, testCase := range copyTests() {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
sourceFiler, sourceDir := tc.setupSource(t)
targetFiler, targetDir := tc.setupTarget(t)
sourceFiler, sourceDir := testCase.setupSource(t)
targetFiler, targetDir := testCase.setupTarget(t)
setupSourceFile(t, context.Background(), sourceFiler)
testcli.RequireSuccessfulRun(t, ctx, "fs", "cp", path.Join(sourceDir, "foo.txt"), targetDir)
@ -205,14 +199,12 @@ func TestFsCpDirToDirFileNotOverwritten(t *testing.T) {
t.Parallel()
for _, testCase := range copyTests() {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
sourceFiler, sourceDir := tc.setupSource(t)
targetFiler, targetDir := tc.setupTarget(t)
sourceFiler, sourceDir := testCase.setupSource(t)
targetFiler, targetDir := testCase.setupTarget(t)
setupSourceDir(t, context.Background(), sourceFiler)
// Write a conflicting file to target
@ -231,14 +223,12 @@ func TestFsCpFileToDirFileNotOverwritten(t *testing.T) {
t.Parallel()
for _, testCase := range copyTests() {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
sourceFiler, sourceDir := tc.setupSource(t)
targetFiler, targetDir := tc.setupTarget(t)
sourceFiler, sourceDir := testCase.setupSource(t)
targetFiler, targetDir := testCase.setupTarget(t)
setupSourceDir(t, context.Background(), sourceFiler)
// Write a conflicting file to target
@ -255,14 +245,12 @@ func TestFsCpFileToFileFileNotOverwritten(t *testing.T) {
t.Parallel()
for _, testCase := range copyTests() {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
sourceFiler, sourceDir := tc.setupSource(t)
targetFiler, targetDir := tc.setupTarget(t)
sourceFiler, sourceDir := testCase.setupSource(t)
targetFiler, targetDir := testCase.setupTarget(t)
setupSourceDir(t, context.Background(), sourceFiler)
// Write a conflicting file to target
@ -279,14 +267,12 @@ func TestFsCpDirToDirWithOverwriteFlag(t *testing.T) {
t.Parallel()
for _, testCase := range copyTests() {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
sourceFiler, sourceDir := tc.setupSource(t)
targetFiler, targetDir := tc.setupTarget(t)
sourceFiler, sourceDir := testCase.setupSource(t)
targetFiler, targetDir := testCase.setupTarget(t)
setupSourceDir(t, context.Background(), sourceFiler)
// Write a conflicting file to target
@ -303,14 +289,12 @@ func TestFsCpFileToFileWithOverwriteFlag(t *testing.T) {
t.Parallel()
for _, testCase := range copyTests() {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
sourceFiler, sourceDir := tc.setupSource(t)
targetFiler, targetDir := tc.setupTarget(t)
sourceFiler, sourceDir := testCase.setupSource(t)
targetFiler, targetDir := testCase.setupTarget(t)
setupSourceDir(t, context.Background(), sourceFiler)
// Write a conflicting file to target
@ -327,14 +311,12 @@ func TestFsCpFileToDirWithOverwriteFlag(t *testing.T) {
t.Parallel()
for _, testCase := range copyTests() {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
sourceFiler, sourceDir := tc.setupSource(t)
targetFiler, targetDir := tc.setupTarget(t)
sourceFiler, sourceDir := testCase.setupSource(t)
targetFiler, targetDir := testCase.setupTarget(t)
setupSourceDir(t, context.Background(), sourceFiler)
// Write a conflicting file to target
@ -351,13 +333,11 @@ func TestFsCpErrorsWhenSourceIsDirWithoutRecursiveFlag(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
_, tmpDir := tc.setupFiler(t)
_, tmpDir := testCase.setupFiler(t)
_, _, err := testcli.RequireErrorRun(t, ctx, "fs", "cp", path.Join(tmpDir), path.Join(tmpDir, "foobar"))
r := regexp.MustCompile("source path .* is a directory. Please specify the --recursive flag")
@ -376,14 +356,12 @@ func TestFsCpSourceIsDirectoryButTargetIsFile(t *testing.T) {
t.Parallel()
for _, testCase := range copyTests() {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
sourceFiler, sourceDir := tc.setupSource(t)
targetFiler, targetDir := tc.setupTarget(t)
sourceFiler, sourceDir := testCase.setupSource(t)
targetFiler, targetDir := testCase.setupTarget(t)
setupSourceDir(t, context.Background(), sourceFiler)
// Write a conflicting file to target

30
integration/cmd/fs/ls_test.go
View File

@ -43,13 +43,11 @@ func TestFsLs(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
f, tmpDir := tc.setupFiler(t)
f, tmpDir := testCase.setupFiler(t)
setupLsFiles(t, f)
stdout, stderr := testcli.RequireSuccessfulRun(t, ctx, "fs", "ls", tmpDir, "--output=json")
@ -77,13 +75,11 @@ func TestFsLsWithAbsolutePaths(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
f, tmpDir := tc.setupFiler(t)
f, tmpDir := testCase.setupFiler(t)
setupLsFiles(t, f)
stdout, stderr := testcli.RequireSuccessfulRun(t, ctx, "fs", "ls", tmpDir, "--output=json", "--absolute")
@ -111,13 +107,11 @@ func TestFsLsOnFile(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
f, tmpDir := tc.setupFiler(t)
f, tmpDir := testCase.setupFiler(t)
setupLsFiles(t, f)
_, _, err := testcli.RequireErrorRun(t, ctx, "fs", "ls", path.Join(tmpDir, "a", "hello.txt"), "--output=json")
@ -131,13 +125,11 @@ func TestFsLsOnEmptyDir(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
_, tmpDir := tc.setupFiler(t)
_, tmpDir := testCase.setupFiler(t)
stdout, stderr := testcli.RequireSuccessfulRun(t, ctx, "fs", "ls", tmpDir, "--output=json")
assert.Equal(t, "", stderr.String())
@ -155,13 +147,11 @@ func TestFsLsForNonexistingDir(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
_, tmpDir := tc.setupFiler(t)
_, tmpDir := testCase.setupFiler(t)
_, _, err := testcli.RequireErrorRun(t, ctx, "fs", "ls", path.Join(tmpDir, "nonexistent"), "--output=json")
assert.ErrorIs(t, err, fs.ErrNotExist)

18
integration/cmd/fs/mkdir_test.go
View File

@ -17,13 +17,11 @@ func TestFsMkdir(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
f, tmpDir := tc.setupFiler(t)
f, tmpDir := testCase.setupFiler(t)
// create directory "a"
stdout, stderr := testcli.RequireSuccessfulRun(t, ctx, "fs", "mkdir", path.Join(tmpDir, "a"))
@ -43,13 +41,11 @@ func TestFsMkdirCreatesIntermediateDirectories(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
f, tmpDir := tc.setupFiler(t)
f, tmpDir := testCase.setupFiler(t)
// create directory "a/b/c"
stdout, stderr := testcli.RequireSuccessfulRun(t, ctx, "fs", "mkdir", path.Join(tmpDir, "a", "b", "c"))
@ -81,13 +77,11 @@ func TestFsMkdirWhenDirectoryAlreadyExists(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
f, tmpDir := tc.setupFiler(t)
f, tmpDir := testCase.setupFiler(t)
// create directory "a"
err := f.Mkdir(context.Background(), "a")

30
integration/cmd/fs/rm_test.go
View File

@ -17,14 +17,12 @@ func TestFsRmFile(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
// Create a file
ctx := context.Background()
f, tmpDir := tc.setupFiler(t)
f, tmpDir := testCase.setupFiler(t)
err := f.Write(context.Background(), "hello.txt", strings.NewReader("abcd"), filer.CreateParentDirectories)
require.NoError(t, err)
@ -48,14 +46,12 @@ func TestFsRmEmptyDir(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
// Create a directory
ctx := context.Background()
f, tmpDir := tc.setupFiler(t)
f, tmpDir := testCase.setupFiler(t)
err := f.Mkdir(context.Background(), "a")
require.NoError(t, err)
@ -79,14 +75,12 @@ func TestFsRmNonEmptyDirectory(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
// Create a directory
ctx := context.Background()
f, tmpDir := tc.setupFiler(t)
f, tmpDir := testCase.setupFiler(t)
err := f.Mkdir(context.Background(), "a")
require.NoError(t, err)
@ -110,13 +104,11 @@ func TestFsRmForNonExistentFile(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
_, tmpDir := tc.setupFiler(t)
_, tmpDir := testCase.setupFiler(t)
// Expect error if file does not exist
_, _, err := testcli.RequireErrorRun(t, ctx, "fs", "rm", path.Join(tmpDir, "does-not-exist"))
@ -129,13 +121,11 @@ func TestFsRmDirRecursively(t *testing.T) {
t.Parallel()
for _, testCase := range fsTests {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
f, tmpDir := tc.setupFiler(t)
f, tmpDir := testCase.setupFiler(t)
// Create a directory
err := f.Mkdir(context.Background(), "a")

12
integration/libs/filer/filer_test.go
View File

@ -128,11 +128,9 @@ func TestFilerRecursiveDelete(t *testing.T) {
{"files", setupUcVolumesFiler},
{"workspace files extensions", setupWsfsExtensionsFiler},
} {
tc := testCase
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
f, _ := tc.f(t)
f, _ := testCase.f(t)
ctx := context.Background()
// Common tests we run across all filers to ensure consistent behavior.
@ -239,11 +237,9 @@ func TestFilerReadWrite(t *testing.T) {
{"files", setupUcVolumesFiler},
{"workspace files extensions", setupWsfsExtensionsFiler},
} {
tc := testCase
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
f, _ := tc.f(t)
f, _ := testCase.f(t)
ctx := context.Background()
// Common tests we run across all filers to ensure consistent behavior.
@ -348,11 +344,9 @@ func TestFilerReadDir(t *testing.T) {
{"files", setupUcVolumesFiler},
{"workspace files extensions", setupWsfsExtensionsFiler},
} {
tc := testCase
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
f, _ := tc.f(t)
f, _ := testCase.f(t)
ctx := context.Background()
commonFilerReadDirTest(t, ctx, f)

3
integration/libs/locker/locker_test.go
View File

@ -66,9 +66,8 @@ func TestLock(t *testing.T) {
}
var wg sync.WaitGroup
for i := range numConcurrentLocks {
for currentIndex := range numConcurrentLocks {
wg.Add(1)
currentIndex := i
go func() {
defer wg.Done()
time.Sleep(time.Duration(rand.Intn(100)) * time.Millisecond)

2
libs/filer/files_client.go
View File

@ -303,8 +303,6 @@ func (w *FilesClient) recursiveDelete(ctx context.Context, name string) error {
group.SetLimit(maxFilesRequestsInFlight)
for _, file := range filesToDelete {
file := file
// Skip the file if the context has already been cancelled.
select {
case <-groupCtx.Done():