many: force-repo-dir instead of force-data-dir

cmd/image-builder/export_test.go

@@ -49,9 +49,9 @@ func MockOsStderr(new io.Writer) (restore func()) {
 
 func MockNewRepoRegistry(f func() (*reporegistry.RepoRegistry, error)) (restore func()) {
 	saved := newRepoRegistry
-	newRepoRegistry = func(dataDir string, extraRepos []string) (*reporegistry.RepoRegistry, error) {
-		if dataDir != "" {
-			panic(fmt.Sprintf("cannot use custom dataDir %v in mock", dataDir))
+	newRepoRegistry = func(repoDir string, extraRepos []string) (*reporegistry.RepoRegistry, error) {
+		if repoDir != "" {
+			panic(fmt.Sprintf("cannot use custom repoDir %v in mock", repoDir))
 		}
 		return f()
 	}

cmd/image-builder/filters.go

@@ -10,9 +10,9 @@ import (
 	"github.com/osbuild/images/pkg/imagefilter"
 )
 
-func newImageFilterDefault(dataDir string, extraRepos []string) (*imagefilter.ImageFilter, error) {
+func newImageFilterDefault(repoDir string, extraRepos []string) (*imagefilter.ImageFilter, error) {
 	fac := distrofactory.NewDefault()
-	repos, err := newRepoRegistry(dataDir, extraRepos)
+	repos, err := newRepoRegistry(repoDir, extraRepos)
 	if err != nil {
 		return nil, err
 	}
@@ -21,8 +21,9 @@ func newImageFilterDefault(dataDir string, extraRepos []string) (*imagefilter.Im
 }
 
 type repoOptions struct {
-	// DataDir contains the base dir for the repo definition search path
-	DataDir string
+	// RepoDir contains the base dir for the repo definition search path, it will also look
+	// in the `repositories` subdirectory to RepoDir
+	RepoDir string
 
 	// ExtraRepos contains extra baseURLs that get added to the depsolving
 	ExtraRepos []string
@@ -37,7 +38,7 @@ func getOneImage(distroName, imgTypeStr, archStr string, repoOpts *repoOptions)
 		repoOpts = &repoOptions{}
 	}
 
-	imageFilter, err := newImageFilterDefault(repoOpts.DataDir, repoOpts.ExtraRepos)
+	imageFilter, err := newImageFilterDefault(repoOpts.RepoDir, repoOpts.ExtraRepos)
 	if err != nil {
 		return nil, err
 	}
@@ -83,7 +84,7 @@ func getAllImages(repoOpts *repoOptions, filterExprs ...string) ([]imagefilter.R
 		repoOpts = &repoOptions{}
 	}
 
-	imageFilter, err := newImageFilterDefault(repoOpts.DataDir, repoOpts.ExtraRepos)
+	imageFilter, err := newImageFilterDefault(repoOpts.RepoDir, repoOpts.ExtraRepos)
 	if err != nil {
 		return nil, err
 	}

cmd/image-builder/list.go

@@ -4,8 +4,8 @@ import (
 	"github.com/osbuild/images/pkg/imagefilter"
 )
 
-func listImages(dataDir string, extraRepos []string, output string, filterExprs []string) error {
-	imageFilter, err := newImageFilterDefault(dataDir, extraRepos)
+func listImages(repoDir string, extraRepos []string, output string, filterExprs []string) error {
+	imageFilter, err := newImageFilterDefault(repoDir, extraRepos)
 	if err != nil {
 		return err
 	}

cmd/image-builder/main.go

@@ -67,7 +67,7 @@ func cmdListImages(cmd *cobra.Command, args []string) error {
 	if err != nil {
 		return err
 	}
-	dataDir, err := cmd.Flags().GetString("force-data-dir")
+	repoDir, err := cmd.Flags().GetString("force-repo-dir")
 	if err != nil {
 		return err
 	}
@@ -76,7 +76,7 @@ func cmdListImages(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	return listImages(dataDir, extraRepos, format, filter)
+	return listImages(repoDir, extraRepos, format, filter)
 }
 
 func ostreeImageOptions(cmd *cobra.Command) (*ostree.ImageOptions, error) {
@@ -146,7 +146,7 @@ func cmdManifestWrapper(pbar progress.ProgressBar, cmd *cobra.Command, args []st
 	if wrapperOpts == nil {
 		wrapperOpts = &cmdManifestWrapperOptions{}
 	}
-	dataDir, err := cmd.Flags().GetString("force-data-dir")
+	repoDir, err := cmd.Flags().GetString("force-repo-dir")
 	if err != nil {
 		return nil, err
 	}
@@ -294,7 +294,7 @@ func cmdManifestWrapper(pbar progress.ProgressBar, cmd *cobra.Command, args []st
 		forceRepos = []string{"https://example.com/not-used"}
 	} else {
 		repoOpts := &repoOptions{
-			DataDir:    dataDir,
+			RepoDir:    repoDir,
 			ExtraRepos: extraRepos,
 			ForceRepos: forceRepos,
 		}
@@ -331,8 +331,7 @@ func cmdManifestWrapper(pbar progress.ProgressBar, cmd *cobra.Command, args []st
 	if opts.ManifestgenOptions.UseBootstrapContainer {
 		fmt.Fprintf(os.Stderr, "WARNING: using experimental cross-architecture building to build %q\n", img.ImgType.Arch().Name())
 	}
-
-	err = generateManifest(dataDir, extraRepos, img, w, opts)
+	err = generateManifest(repoDir, extraRepos, img, w, opts)
 	return img, err
 }
 
@@ -467,7 +466,7 @@ func cmdBuild(cmd *cobra.Command, args []string) error {
 
 func cmdDescribeImg(cmd *cobra.Command, args []string) error {
 	// XXX: boilderplate identical to cmdManifest() above
-	dataDir, err := cmd.Flags().GetString("force-data-dir")
+	repoDir, err := cmd.Flags().GetString("force-repo-dir")
 	if err != nil {
 		return err
 	}
@@ -489,7 +488,7 @@ func cmdDescribeImg(cmd *cobra.Command, args []string) error {
 	}
 
 	imgTypeStr := args[0]
-	res, err := getOneImage(distroStr, imgTypeStr, archStr, &repoOptions{DataDir: dataDir})
+	res, err := getOneImage(distroStr, imgTypeStr, archStr, &repoOptions{RepoDir: repoDir})
 	if err != nil {
 		return err
 	}
@@ -500,7 +499,10 @@ func cmdDescribeImg(cmd *cobra.Command, args []string) error {
 func normalizeRootArgs(_ *pflag.FlagSet, name string) pflag.NormalizedName {
 	switch name {
 	case "data-dir":
-		name = "force-data-dir"
+		name = "force-repo-dir"
+		break
+	case "force-data-dir":
+		name = "force-repo-dir"
 		break
 	}
 
@@ -536,10 +538,12 @@ operating systems like Fedora, CentOS and RHEL with easy customizations support.
 	}
 
 	rootCmd.Flags().Bool("version", false, "Print version information and exit")
-	var forceDataDir string
-	rootCmd.PersistentFlags().StringVar(&forceDataDir, "force-data-dir", "", `Override the default data directory for e.g. custom repositories/*.json data`)
-	rootCmd.PersistentFlags().StringVar(&forceDataDir, "data-dir", "", `Override the default data directory for e.g. custom repositories/*.json data`)
-	rootCmd.PersistentFlags().MarkDeprecated("data-dir", `Use --force-data-dir instead`)
+	var forceRepoDir string
+	rootCmd.PersistentFlags().StringVar(&forceRepoDir, "force-repo-dir", "", "Override the default repository search path for custom repository files")
+	rootCmd.PersistentFlags().StringVar(&forceRepoDir, "force-data-dir", "", `Override the default data directory for e.g. custom repositories/*.json data`)
+	rootCmd.PersistentFlags().MarkDeprecated("force-data-dir", `Use --force-repo-dir instead`)
+	rootCmd.PersistentFlags().StringVar(&forceRepoDir, "data-dir", "", `Override the default data directory for e.g. custom repositories/*.json data`)
+	rootCmd.PersistentFlags().MarkDeprecated("data-dir", `Use --force-repo-dir instead`)
 	rootCmd.PersistentFlags().StringArray("extra-repo", nil, `Add an extra repository during build (will *not* be gpg checked and not be part of the final image)`)
 	rootCmd.PersistentFlags().StringArray("force-repo", nil, `Override the base repositories during build (these will not be part of the final image)`)
 	rootCmd.PersistentFlags().String("output-dir", "", `Put output into the specified directory`)

cmd/image-builder/manifest.go

@@ -57,8 +57,8 @@ func sbomWriter(outputDir, filename string, content io.Reader) error {
 }
 
 // XXX: just return []byte instead of using output writer
-func generateManifest(dataDir string, extraRepos []string, img *imagefilter.Result, output io.Writer, opts *manifestOptions) error {
-	repos, err := newRepoRegistry(dataDir, extraRepos)
+func generateManifest(repoDir string, extraRepos []string, img *imagefilter.Result, output io.Writer, opts *manifestOptions) error {
+	repos, err := newRepoRegistry(repoDir, extraRepos)
 	if err != nil {
 		return err
 	}

cmd/image-builder/repos.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"io/fs"
 	"net/url"
+	"os"
 	"path/filepath"
 
 	"github.com/osbuild/images/data/repositories"
@@ -53,12 +54,18 @@ func parseRepoURLs(repoURLs []string, what string) ([]rpmmd.RepoConfig, error) {
 	return repoConf, nil
 }
 
-func newRepoRegistryImpl(dataDir string, extraRepos []string) (*reporegistry.RepoRegistry, error) {
+func newRepoRegistryImpl(repoDir string, extraRepos []string) (*reporegistry.RepoRegistry, error) {
 	var repoDirs []string
 	var builtins []fs.FS
 
-	if dataDir != "" {
-		repoDirs = []string{filepath.Join(dataDir, "repositories")}
+	if repoDir != "" {
+		withRepoSubdir := filepath.Join(repoDir, "repositories")
+		if _, err := os.Stat(withRepoSubdir); err == nil {
+			// we don't care about the error case here, we just want to know
+			// if it exists; not if we can't read it or other errors
+			fmt.Fprintf(os.Stderr, "WARNING: found a `repositories` subdirectory at '%s', in the future `image-builder` will not descend into this subdirectory to look for repository files. Please move any repository files directly into the directory '%s' and remove the `repositories` subdirectory to silence this warning.\n", withRepoSubdir, repoDir)
+		}
+		repoDirs = []string{withRepoSubdir, repoDir}
 	} else {
 		repoDirs = defaultRepoDirs
 		builtins = []fs.FS{repos.FS}

cmd/image-builder/repos_test.go

@@ -61,18 +61,18 @@ func TestNewRepoRegistryImplExtraReposGetAppended(t *testing.T) {
 	assert.Equal(t, repos[len(repos)-1].BaseURLs[0], "https://example.com/my/repo")
 }
 
-func TestNewRepoRegistryImplDatadir(t *testing.T) {
+func TestNewRepoRegistryImplRepodir(t *testing.T) {
 	// prereq test: no testdistro-1 in the default repos
 	registry, err := newRepoRegistryImpl("", nil)
 	require.NoError(t, err)
 	assert.NotContains(t, registry.ListDistros(), "testdistro-1")
 	_, err = registry.DistroHasRepos("testdistro-1", "x86_64")
 	require.EqualError(t, err, `requested repository not found: for distribution "testdistro-1"`)
 
-	// create a custom datadir with testdistro-1.json, the basefilename
+	// create a custom repodir with testdistro-1.json, the basefilename
 	// must match a distro nameVer
-	dataDir := t.TempDir()
-	repoFile := filepath.Join(dataDir, "repositories", "testdistro-1.json")
+	repoDir := t.TempDir()
+	repoFile := filepath.Join(repoDir, "repositories", "testdistro-1.json")
 	err = os.Mkdir(filepath.Dir(repoFile), 0755)
 	require.NoError(t, err)
 	repoContents := `{
@@ -88,7 +88,40 @@ func TestNewRepoRegistryImplDatadir(t *testing.T) {
 	require.NoError(t, err)
 
 	// and ensure we have testdistro-1 now
-	registry, err = newRepoRegistryImpl(dataDir, nil)
+	registry, err = newRepoRegistryImpl(repoDir, nil)
+	require.NoError(t, err)
+	repos, err := registry.DistroHasRepos("testdistro-1", "x86_64")
+	require.NoError(t, err)
+	assert.Len(t, repos, 1)
+	assert.Equal(t, repos[0].Name, "testdistro-1-repo")
+}
+
+func TestNewRepoRegistryImplRepodirNoSubDir(t *testing.T) {
+	// prereq test: no testdistro-1 in the default repos
+	registry, err := newRepoRegistryImpl("", nil)
+	require.NoError(t, err)
+	assert.NotContains(t, registry.ListDistros(), "testdistro-1")
+	_, err = registry.DistroHasRepos("testdistro-1", "x86_64")
+	require.EqualError(t, err, `requested repository not found: for distribution "testdistro-1"`)
+
+	// create a custom repodir with testdistro-1.json, the basefilename
+	// must match a distro nameVer
+	repoDir := t.TempDir()
+	repoFile := filepath.Join(repoDir, "testdistro-1.json")
+	repoContents := `{
+	"x86_64": [
+		{
+			"name": "testdistro-1-repo",
+			"baseurl": "https://example.com/test/test/distro/1"
+		}
+	]
+}
+`
+	err = os.WriteFile(repoFile, []byte(repoContents), 0644)
+	require.NoError(t, err)
+
+	// and ensure we have testdistro-1 now
+	registry, err = newRepoRegistryImpl(repoDir, nil)
 	require.NoError(t, err)
 	repos, err := registry.DistroHasRepos("testdistro-1", "x86_64")
 	require.NoError(t, err)

test/test_smoke.py

@@ -142,7 +142,8 @@ def test_progress_smoke(tmp_path, build_fake_container, case: ProgressTestCase):
 def test_smoke_force_data_dir(tmp_path, build_container):
     """
     Ensure that when a data dir is passed through `--force-data-dir` that only
-    distributions with repository files inside that directory are available.
+    distributions with repository files inside that directory are available and
+    that warnings are emitted.
 
     Note that there's no 'negative' test case to this one, the default state of no
     data directory is already tested by the other smoke tests.
@@ -162,7 +163,43 @@ def test_smoke_force_data_dir(tmp_path, build_container):
         build_container,
         "--force-data-dir", "/data",
         "list",
-    ], text=True)
+    ], stderr=subprocess.STDOUT, text=True)
+
+    lines = output.splitlines()
+
+    # assert that a warning is emitted (deprecated argument!)
+    assert "has been deprecated" in lines[0]
+
+    # assert that a warning is emitted (repositories subdirectory!)
+    assert "move any repository files" in lines[1]
+
+    # ensure the rest of the lines all contain `rhel-10.0`
+    assert all("rhel-10.0" in line for line in lines[2:])
+
+
+def test_smoke_force_repo_dir(tmp_path, build_container):
+    """
+    Ensure that when a repo dir is passed through `--force-repo-dir` that only
+    distributions with repository files inside that directory are available.
+
+    Note that there's no 'negative' test case to this one, the default state of no
+    data directory is already tested by the other smoke tests.
+    """
+
+    repodir = pathlib.Path(tmp_path)
+
+    (repodir / "rhel-10.0.json").write_text(json.dumps({
+        "x86_64": [
+            {"name": "test", "baseurl": "test"},
+        ],
+    }))
+
+    output = subprocess.check_output(podman_run + [
+        "-v", f"{tmp_path!s}:/data",
+        build_container,
+        "--force-repo-dir", "/data",
+        "list",
+    ], stderr=subprocess.STDOUT, text=True)
 
-    # ensure we only have lines containing `rhel-10.0`
+    # ensure the rest of the lines all contain `rhel-10.0`
     assert all("rhel-10.0" in line for line in output.splitlines())