WithEmbeddedFileSystem

[piotrkowalczuk]

WithEmbeddedFileSystem adds ability to fallback to embedded file system if given configuration file was no found on the host.

[peterbourgon]

What is the use case here?

[piotrkowalczuk]

This PR is motivated by my situation. I must migrate some apps to an opinionated, security-centric environment with limited options to alter CI/CD pipelines. It might benefit everyone who, for some reason, is forced to distribute their apps as a single binary.

[peterbourgon]

Got it, so you have a config file in an embedded FS that's compiled into the binary? A config file that you can't change at runtime is a bit odd, right? In any case, I could be convinced of this, but with a different approach: refactoring the current implementation to read files thru an abstract FS, which would have a default implementation of the host's true filesystem, and could be swapped for an embedded FS with an option. Up for doing it that way?

[piotrkowalczuk]

A config file that you can't change at runtime is a bit odd, right?

It's last line of defense, it works like a default values for flags:

flag.IntVar(&flagvar, "flagname", 1234, "help message for flagname")

So overall order remains intact:

• flags
• env vars
• config file
• embedded config file (new)

I could be convinced of this, but with a different approach: refactoring the current implementation to read files thru an abstract FS, which would have a default implementation of the host's true filesystem, and could be swapped for an embedded FS with an option. Up for doing it that way?

Sure thing.

[piotrkowalczuk]

Up

[peterbourgon]

Thanks for your persistence (really).

The core of the change appears to be this patch to the Parse function:

  if parseConfigFile {
- 		f, err := os.Open(configFile)
+ 		f, err := c.fileSystem.Open(configFile)

which means that the config file will be loaded thru c.fileSystem no matter what it is — os.Open by default, embed.FS.Open when so specified. But that seems incongruous with the comment on WithEmbeddedFileSystem, which says that it "tells Parse to fallback to embedded data" only "if a given configuration file cannot be found in the host file system." Which is correct?

I think I'm still a bit confused on the motivating use case, to be honest. Can you say anything more about the situation you have which is motivating this change? I'm not looking for trade secrets 😉 but rather the specific requirements and constraints of the runtime environment where this feature would provide value. Is the binary built with a specific embedded FS containing a config file that you want to use? Is the binary run in an environment where it doesn't have access to a local file system at all?

Thank you for bearing with me, again.

[peterbourgon]

(cc: @piotrkowalczuk)

[piotrkowalczuk]

which means that the config file will be loaded thru c.fileSystem no matter what it is

Good catch, let me fix that.

Is the binary built with a specific embedded FS containing a config file that you want to use?
That's correct.

Is the binary run in an environment where it doesn't have access to a local file system at all?

Not completely, but the CI/CD pipeline is very opinionated, and it strictly defines what can be shipped:

• imposed generic docker image
• ability to define just a subset of k8s deployment manifest

[peterbourgon]

Alright, I think I understand, at least at a high level.

What do you think of this patch?

diff --git a/parse.go b/parse.go
index 0c53daf..88a6787 100644
--- a/parse.go
+++ b/parse.go
@@ -1,9 +1,12 @@
 package ff
 
 import (
+	"embed"
+	"errors"
 	"flag"
 	"fmt"
 	"io"
+	iofs "io/fs"
 	"os"
 	"strings"
 )
@@ -103,13 +106,19 @@ func Parse(fs *flag.FlagSet, args []string, options ...Option) error {
 		}
 	}
 
+	if c.configFileOpenFunc == nil {
+		c.configFileOpenFunc = func(s string) (iofs.File, error) {
+			return os.Open(s)
+		}
+	}
+
 	var (
 		haveConfigFile  = configFile != ""
 		haveParser      = c.configFileParser != nil
 		parseConfigFile = haveConfigFile && haveParser
 	)
 	if parseConfigFile {
-		f, err := os.Open(configFile)
+		f, err := c.configFileOpenFunc(configFile)
 		switch {
 		case err == nil:
 			defer f.Close()
@@ -151,7 +160,7 @@ func Parse(fs *flag.FlagSet, args []string, options ...Option) error {
 				return err
 			}
 
-		case os.IsNotExist(err) && c.allowMissingConfigFile:
+		case errors.Is(err, iofs.ErrNotExist) && c.allowMissingConfigFile:
 			// no problem
 
 		default:
@@ -172,6 +181,7 @@ type Context struct {
 	configFileFlagName     string
 	configFileParser       ConfigFileParser
 	configFileLookup       lookupFunc
+	configFileOpenFunc     func(string) (iofs.File, error)
 	allowMissingConfigFile bool
 	readEnvVars            bool
 	envVarPrefix           string
@@ -278,6 +288,15 @@ func WithIgnoreUndefined(ignore bool) Option {
 	}
 }
 
+// WithFilesystem tells Parse to use the provided filesystem when accessing
+// files on disk, which typically means when accessing config files. By default,
+// the host filesystem (via package os) is used.
+func WithFilesystem(fs embed.FS) Option {
+	return func(c *Context) {
+		c.configFileOpenFunc = fs.Open
+	}
+}
+
 var flagNameToEnvVar = strings.NewReplacer(
 	"-", "_",
 	".", "_",

[piotrkowalczuk]

Works for me :)