-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WithEmbeddedFileSystem #103
Conversation
What is the use case here? |
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. |
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? |
It's last line of defense, it works like a default values for flags:
So overall order remains intact:
Sure thing. |
c9fcc90
to
b177c7f
Compare
Up |
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. |
(cc: @piotrkowalczuk) |
Good catch, let me fix that.
Not completely, but the CI/CD pipeline is very opinionated, and it strictly defines what can be shipped:
|
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(
"-", "_",
".", "_", |
Works for me :) |
WithEmbeddedFileSystem
adds ability to fallback to embedded file system if given configuration file was no found on the host.