fix: race condition and remove unused code (#273)

* refactor: small code improvements

Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>

* refactor: small code improvements

Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>

* fix: test

Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>

* test: race condition

Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>

* fix: race

Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>

* test: fix invalid file

Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>

* docs: fix synlinks/indent

Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

files/files.go

@@ -1,15 +1,12 @@
 package files
 
 import (
-	"fmt"
 	"os"
 	"path/filepath"
 	"sort"
 	"time"
 
 	"github.com/goreleaser/fileglob"
-	"gopkg.in/yaml.v3"
-
 	"github.com/goreleaser/nfpm/v2/internal/glob"
 )
 
@@ -34,44 +31,6 @@ type ContentFileInfo struct {
 // Contents list of Content to process.
 type Contents []*Content
 
-type simpleContents map[string]string
-
-func (c *Contents) UnmarshalYAML(value *yaml.Node) (err error) {
-	// nolint:exhaustive
-	// we do not care about `AliasNode`, `DocumentNode`
-	switch value.Kind {
-	case yaml.SequenceNode:
-		type tmpContents Contents
-		var tmp tmpContents
-		if err = value.Decode(&tmp); err != nil {
-			return err
-		}
-		*c = Contents(tmp)
-	case yaml.MappingNode:
-		var tmp simpleContents
-		if err = value.Decode(&tmp); err != nil {
-			return err
-		}
-		for src, dst := range tmp {
-			*c = append(*c, &Content{
-				Source:      src,
-				Destination: dst,
-			})
-		}
-	case yaml.ScalarNode:
-		// TODO: implement issue-43 here and remove the fallthrough
-		// nolint:gocritic
-		// ignoring `emptyFallthrough: remove empty case containing only fallthrough to default case`
-		fallthrough
-	default:
-		// nolint:goerr113
-		// this is temporary so we do not need the a static error
-		return fmt.Errorf("not implemented")
-	}
-
-	return nil
-}
-
 func (c Contents) Len() int {
 	return len(c)
 }
@@ -91,25 +50,33 @@ func (c Contents) Less(i, j int) bool {
 	return a.Destination < b.Destination
 }
 
-func (c *Content) WithFileInfoDefaults() {
-	if c.FileInfo == nil {
-		c.FileInfo = &ContentFileInfo{}
+func (c *Content) WithFileInfoDefaults() *Content {
+	var cc = &Content{
+		Source:      c.Source,
+		Destination: c.Destination,
+		Type:        c.Type,
+		Packager:    c.Packager,
+		FileInfo:    c.FileInfo,
 	}
-	if c.FileInfo.Owner == "" {
-		c.FileInfo.Owner = "root"
+	if cc.FileInfo == nil {
+		cc.FileInfo = &ContentFileInfo{}
 	}
-	if c.FileInfo.Group == "" {
-		c.FileInfo.Group = "root"
+	if cc.FileInfo.Owner == "" {
+		cc.FileInfo.Owner = "root"
 	}
-	info, err := os.Stat(c.Source)
+	if cc.FileInfo.Group == "" {
+		cc.FileInfo.Group = "root"
+	}
+	info, err := os.Stat(cc.Source)
 	if err == nil {
-		c.FileInfo.MTime = info.ModTime()
-		c.FileInfo.Mode = info.Mode()
-		c.FileInfo.Size = info.Size()
+		cc.FileInfo.MTime = info.ModTime()
+		cc.FileInfo.Mode = info.Mode()
+		cc.FileInfo.Size = info.Size()
 	}
-	if c.FileInfo.MTime.IsZero() {
-		c.FileInfo.MTime = time.Now().UTC()
+	if cc.FileInfo.MTime.IsZero() {
+		cc.FileInfo.MTime = time.Now().UTC()
 	}
+	return cc
 }
 
 // Name to part of the os.FileInfo interface
@@ -153,8 +120,7 @@ func ExpandContentGlobs(filesSrcDstMap Contents, disableGlobbing bool) (files Co
 		switch f.Type {
 		case "ghost", "symlink":
 			// Ghost and symlink files need to be in the list, but dont glob them because they do not really exist
-			f.WithFileInfoDefaults()
-			files = append(files, f)
+			files = append(files, f.WithFileInfoDefaults())
 			continue
 		}
 
@@ -181,8 +147,7 @@ func appendGlobbedFiles(globbed map[string]string, origFile *Content, incFiles C
 			FileInfo:    origFile.FileInfo,
 			Packager:    origFile.Packager,
 		}
-		newFile.WithFileInfoDefaults()
-		files = append(files, newFile)
+		files = append(files, newFile.WithFileInfoDefaults())
 	}
 
 	return files

files/files_test.go

@@ -2,6 +2,7 @@ package files_test
 
 import (
 	"strings"
+	"sync"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -24,9 +25,9 @@ contents:
 - src: a
   dst: b
   type: "config|noreplace"
+  packager: "rpm"
   file_info:
     mode: 0644
-    packager: "rpm"
     mtime: 2008-01-02T15:04:05Z
 `))
 	dec.KnownFields(true)
@@ -40,31 +41,24 @@ contents:
 	}
 }
 
-func TestMapperDecode(t *testing.T) {
+func TestRace(t *testing.T) {
 	var config testStruct
 	dec := yaml.NewDecoder(strings.NewReader(`---
 contents:
-  a: b
-  a2: b2
+- src: a
+  dst: b
+  type: symlink
 `))
-	dec.KnownFields(true)
 	err := dec.Decode(&config)
 	require.NoError(t, err)
-	assert.Len(t, config.Contents, 2)
-	for _, f := range config.Contents {
-		t.Logf("%+#v\n", f)
-		assert.Equal(t, f.Packager, "")
-		assert.Equal(t, f.Type, "")
+	var wg sync.WaitGroup
+	for i := 0; i < 10; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			_, err := files.ExpandContentGlobs(config.Contents, false)
+			require.NoError(t, err)
+		}()
 	}
-}
-
-func TestStringDecode(t *testing.T) {
-	var config testStruct
-	dec := yaml.NewDecoder(strings.NewReader(`---
-contents: /path/to/a/tgz
-`))
-	dec.KnownFields(true)
-	err := dec.Decode(&config)
-	require.Error(t, err)
-	assert.Equal(t, err.Error(), "not implemented")
+	wg.Wait()
 }

nfpm.go

@@ -305,8 +305,13 @@ func Validate(info *Info) (err error) {
 		return ErrFieldEmpty{"version"}
 	}
 
-	info.Contents, err = files.ExpandContentGlobs(info.Contents, info.DisableGlobbing)
-	return err
+	contents, err := files.ExpandContentGlobs(info.Contents, info.DisableGlobbing)
+	if err != nil {
+		return err
+	}
+
+	info.Contents = contents
+	return nil
 }
 
 // WithDefaults set some sane defaults into the given Info.

testdata/contents.yaml

@@ -10,18 +10,18 @@ contents:
   - src: "./testdata/whatever2.conf"
     dst: "/etc/rpm/whatever.conf"
     type: "config|noreplace"
+    packager: "rpm"
     file_info:
       mode: 0644
-      packager: "rpm"
   - src: "./testdata/whatever2.conf"
     dst: "/etc/apk/whatever.conf"
     type: "config|noreplace"
+    packager: "apk"
     file_info:
       mode: 0644
-      packager: "apk"
   - src: "./testdata/whatever2.conf"
     dst: "/etc/deb/whatever.conf"
     type: "config|noreplace"
+    packager: "deb"
     file_info:
       mode: 0644
-      packager: "deb"

www/docs/configuration.md

@@ -85,62 +85,63 @@ suggests:
 # Packages it conflicts with. (overridable)
 conflicts:
   - mercurial
-    
+
 # Contents to add to the package
 # This can be binaries or any other files.
 contents:
-    # Basic file that applies to all packagers
+  # Basic file that applies to all packagers
   - src: path/to/local/foo
     dst: /usr/local/bin/foo
-    
-    # Simple config file
+
+  # Simple config file
   - src: path/to/local/foo.conf
     dst: /etc/foo.conf
     type: config
-    
-    # Simple symlink
-  - src: /sbin/foo # link name
-    dst: /usr/local/bin/foo # real location
-    type: "symlink"
-    
-    # Corresponds to %config(noreplace) if the packager is rpm, otherwise it is just a config file
+
+  # Simple symlink
+  #	Will result in `ln -s /sbin/foo /usr/local/bin/foo`.
+  - src: /sbin/foo
+    dst: /usr/local/bin/foo
+    type: symlink
+
+  # Corresponds to `%config(noreplace)` if the packager is rpm, otherwise it is just a config file
   - src: path/to/local/bar.conf
     dst: /etc/bar.conf
-    type: "config|noreplace"
+    type: config|noreplace
 
-    # These files are not actually present in the package, but the file names
-    # are added to the package header. From the RPM directives documentation:
-    #
-    # "There are times when a file should be owned by the package but not
-    # installed - log files and state files are good examples of cases you might
-    # desire this to happen."
-    #
-    # "The way to achieve this, is to use the %ghost directive. By adding this
-    # directive to the line containing a file, RPM will know about the ghosted
-    # file, but will not add it to the package."
-    # 
-    # For non rpm packages ghost files are ignored at this time.
+  # These files are not actually present in the package, but the file names
+  # are added to the package header. From the RPM directives documentation:
+  #
+  # "There are times when a file should be owned by the package but not
+  # installed - log files and state files are good examples of cases you might
+  # desire this to happen."
+  #
+  # "The way to achieve this, is to use the %ghost directive. By adding this
+  # directive to the line containing a file, RPM will know about the ghosted
+  # file, but will not add it to the package."
+  #
+  # For non rpm packages ghost files are ignored at this time.
   - dst: /etc/casper.conf
     type: ghost
   - dst: /var/log/boo.log
     type: ghost
-    
-    # You can user the packager field to add files that are unique to a specific packager 
+
+  # You can user the packager field to add files that are unique to a specific packager
   - src: path/to/rpm/file.conf
     dst: /etc/file.conf
-    type: "config|noreplace"
+    type: config|noreplace
     packager: rpm
   - src: path/to/deb/file.conf
     dst: /etc/file.conf
-    type: "config|noreplace"
+    type: config|noreplace
     packager: deb
   - src: path/to/apk/file.conf
     dst: /etc/file.conf
-    type: "config|noreplace"
+    type: config|noreplace
     packager: apk
-    
-    # Sometimes it is important to be able to set the mtime, mode, owner, or group for a file 
-    # that differs from what is on the local build system at build time.
+
+  # Sometimes it is important to be able to set the mtime, mode, owner, or group for a file
+  # that differs from what is on the local build system at build time.
   - src: path/to/foo
     dst: /usr/local/foo
     file_info: