[jsalaber]

Summary

• init command creates a .openfeature.yaml to store flagSourceUrl using the --flag-source-url
• adds a new pull command that fetches a flag configuration from a flagSourceUrl configured in the init command
• pull command prompts the user for default values if the source configuration doesn't define default values
• overwrites the manifest with the source url flags
• supports using local file uri or http: file:// or https://

Related Issues

#3

Notes

• refactored out the Load function from flagset due to circular dependency when trying to use flagset type in the manifest package
• new LoadFromSourceFlags takes the type from a source configuration and compares it to different types that could match, e.g. Boolean, bool, boolean

Todo

• Support different auth types other than auth token as a header, and figure out a way to store it securely without needing to pass it in as a flag in the pull command

[dominikhaska]

I think it would be good to add some tests - there is an in memory filesystem in generate_test.go so it could be used and probably faking network should not be that hard.

[jsalaber]

@dominikhaska added a couple of tests for the pull command!

[kriscoleman]

this is looking great, thanks!
I think this is a great first step to push/pull/diff.

I was also ideating on a pull command too, and had wondered what kind of sources we might pull from.
Just food for thought, I imagined we might have a concept of Sources, a source might be as simple as a public URL to a manifest file, or an authed endpoint like here, a file stored in git, or from an instance of openfeature itself. We might be able to contribute different source provider types (GitSource, OpenFeatureApiSource, URLSource, etc.), to make this extensible and able to encapsulate domain specifics for different sources.

[beeme1mr]

I was also ideating on a pull command too, and had wondered what kind of sources we might pull from.
Just food for thought, I imagined we might have a concept of Sources, a source might be as simple as a public URL to a manifest file, or an authed endpoint like here, a file stored in git, or from an instance of openfeature itself. We might be able to contribute different source provider types (GitSource, OpenFeatureApiSource, URLSource, etc.), to make this extensible and able to encapsulate domain specifics for different sources.

I was thinking about that too. Perhaps we could do something similar to flagd where we accept a --uri flag and use the protocol identifier to determine how to pull from the source. We could start by supporting only http, but we could easily add support for git, ssh, s3, and more.

[beeme1mr]

I think the flag source object should look like this:

{
  "flags": [
     {
        "key" "test",
        "type": "boolean",
        "defaultValue": false
     }
  ]
}

This will allow us to add additional top-level properties without requiring a breaking change.

[jsalaber]

Do you mean the source should return with a top level flags property or when we unmarshal the data to pass around a data object that has the flags property?

[beeme1mr]

The source is what I had in mind.

[kriscoleman]

Thanks for contributing and updating this PR!

I gave it a look and left a couple comments and ideas I had.

I also noticed there are some CI and Lint errors, FYI.

[kriscoleman]

I do wonder about this prompting. I understand this is likely introduced to handle an edge case where the manifest upstream is lacking defaultValues.

I'm thinking ahead a bit to using this in the github action we've been proposing: When the gha attempts to use pull, if it prompted the user for default values, it would hang. It's a machine workflow at that point and this logic won't support that.

I'm wondering if the prompting could be enabled/disabled if we feel strongly this should be here.

Alternatively, maybe we make defaultValues required and have validation fail if they are missing (I lean this way IMHO)

[kriscoleman]

this struct seems redundant, is there already a struct we could reuse?

[kriscoleman]

similarily, this bit of logic feels redundant too. It looks like this func trying to do a lot of the same things as UnmarshalJSON above. Could we consolidate?

[kriscoleman]

I think these changes introduce a leaky abstraction. Now, callers of LoadFromSourceFlags like cmd/pull.go are burdened with implementation details (e.g., how the data is fetched and passed in), rather than simply asking for a manifest to be loaded.

I’d suggest preserving encapsulation and consolidating code. This func could be repurposed to LoadFromLocal(manifestPath string) and could delegate to your new fileSystem.ReadFile() logic here, under the hood. This maintains the clean API contract for file-based usage, and leverages the reuse you added to fileSystem. Additionally, consider adding a LoadFromRemote(sourceUrl string, authToken string) function here to encapsulate remote retrieval logic. With this in place, requests/fetchFlags.go could be eliminated entirely, consolidating responsibilities inside this module where they belong.

I think then LoadFromRemote could reuse the UnmarshalJSON func, or the LoadFromSourceFlags func, but we should only have one of those funcs in flagset.go, not both.

I think after this, we might end up with a smaller PR, and we'd simplify the logic for cmd/pull.go, so it might instead look something like:

if strings.HasPrefix(flagSourceUrl, "file://") {
	loadedFlags, err := manifest.LoadFromLocal(flagSourceUrl)
	if err != nil {
		return fmt.Errorf("error loading flags from local file: %w", err)
	}
	flags.Flags = *loadedFlags
} else if strings.HasPrefix(flagSourceUrl, "http://") && !strings.HasPrefix(flagSourceUrl, "https://") {
	flags, err = manifest.LoadFromRemote(flagSourceUrl, authToken)
	if err != nil {
		return fmt.Errorf("error reading remote flags file: %w", err)
	}
	return nil
}

We could actually put the logic above into a super-func that encapsulates both load methods for callers, which I think would make the API even better, too:

// in manifest.go
func Load(flagSourceUrl string) (*flagset.Flagset, error) {
	if strings.HasPrefix(flagSourceUrl, "file://") {
		loadedFlags, err := LoadFromLocal(flagSourceUrl)
		if err != nil {
			return nil, fmt.Errorf("error loading flags from local file: %w", err)
		}
		return loadedFlags, nil
	} else if strings.HasPrefix(flagSourceUrl, "http://") && !strings.HasPrefix(flagSourceUrl, "https://") {
		flags, err := LoadFromRemote(flagSourceUrl, authToken)
		if err != nil {
			return nil, fmt.Errorf("error reading remote flags: %w", err)
		}
		return flags, nil
	}
	return nil, fmt.Errorf("unsupported flag source URL: %s", flagSourceUrl)
}

// in pull.go
flags.Flags, err = manifest.Load(flagSourceUrl)
if err != nil {
	log.Fatalf("failed to load flags: %v", err)
}

My $0.02 cents for reasoning:
Encapsulation simplifies the API surface for consumers and aligns with good separation of concerns. By internalizing the mechanics of how and where flags are loaded (local file vs remote URL), we avoid leaking implementation details and reduce the likelihood of misuse or duplication of implementation details.

[kriscoleman]

One final thought: we should ensure we also use the validation logic for both the Load from file, and the Load from remote. I could be missing it, but it looks like the remote pull does not perform validation with these current changes.

[kriscoleman]

is this a local flags file we're logging an error for, or a remote file?

[kriscoleman]

I contributed the feedback from this pr to #147

[beeme1mr]

Closing in favor of #147