Skip to content

feat: Create openfeature pull command - #75

Closed
jsalaber wants to merge 3 commits into
open-feature:refactor-manifestfrom
jsalaber:feat/openfeature-pull
Closed

feat: Create openfeature pull command#75
jsalaber wants to merge 3 commits into
open-feature:refactor-manifestfrom
jsalaber:feat/openfeature-pull

Conversation

@jsalaber

@jsalaber jsalaber commented Mar 12, 2025

Copy link
Copy Markdown
Contributor

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

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

@jsalaber
jsalaber force-pushed the feat/openfeature-pull branch 3 times, most recently from db4605c to 4c02198 Compare March 12, 2025 18:04
@beeme1mr

beeme1mr commented Mar 12, 2025

Copy link
Copy Markdown
Member

For anyone interested in testing this locally:

go run main.go pull --flagSourceUrl https://gist.githubusercontent.com/beeme1mr/f74bc892c9651d029fa804657e847237/raw/b1a21304df3a9b99e13c4c91b42245e56d90dffa/flags

Note

You also need to comment out the auth header on line 17 in fetchFlags.go.

Comment thread cmd/pull.go
return map[string]any{}
default:
return nil
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the UX here, well done 🥇

case "flagSourceUrl":
return config.FlagSourceUrl, nil
default:
return "", fmt.Errorf("unknown key: %s", key)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we could drop the switch statements if we take the map from unmarshal and do a key/value lookup on it.

Then maybe we could return with something like this:

	value, exists := config[key]
	if !exists {
		return "", fmt.Errorf("unknown key: %s", key)
	}

	return value, nil
	```

Comment thread internal/manifest/manage.go Outdated
}

// Loads, validates, and unmarshals the manifest file at the given path into a flagset
func Load(manifestPath string) (*flagset.Flagset, error) {

@kriscoleman kriscoleman Mar 13, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call, I think moving this logic here makes sense 👍

EDIT: on second thought, I mistakenly thought this returned a Manifest obj, and I was going to use it in my branch, but it looks like it loads the manifest from path into a flagset obj.

I would expect if I call manifest.Load() it would return a manifest, just some food for thought.

Perhaps we could keep this in flagset, and I would suggest maybe naming it flagset.LoadFromManifest; or, if we keep it here, I suggest naming it manifest.LoadFlagset()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a good point, the problem I had keeping it in the flagset file was a circular dependency occurring where I needed the type defined in flagset in this package, but the flagset file also imported the manifest package

I wouldn't mind renaming this to be more specific to loading the flagset though!

@kriscoleman

Copy link
Copy Markdown
Collaborator

looking great to me so far 😄

Signed-off-by: Jason Salaber <jcsalaber@hotmail.com>
@jsalaber
jsalaber force-pushed the feat/openfeature-pull branch from 4c02198 to 1204f4f Compare March 14, 2025 20:01
@beeme1mr
beeme1mr deleted the branch open-feature:refactor-manifest March 14, 2025 20:12
@beeme1mr beeme1mr closed this Mar 14, 2025
@beeme1mr

Copy link
Copy Markdown
Member

Sorry @jsalaber, this automatically closed when I merged my other PR but I'm not sure why. Could you please reopen it when you have moment?

@jsalaber

Copy link
Copy Markdown
Contributor Author

@beeme1mr yep, recreating it against main

jonathannorris added a commit that referenced this pull request Aug 17, 2026
- @angular/core 20.3.25 -> 20.3.28 (high, alert #75: XSS via event-handler attributes)
- @angular/compiler 20.3.25 -> 20.3.28 (high, alert #74: XSS via event-handler attributes)
- @angular/common 20.3.25 -> 20.3.28 (high, alert #73: cache-key ambiguity in HttpTransferCache)
- brace-expansion 2.1.0 -> 2.1.4 (high, alert #72: DoS via exponential-time expansion)

Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
jonathannorris added a commit that referenced this pull request Aug 17, 2026
* chore: resolve open dependabot security alerts

- vite ^6.4.2 -> ^6.4.3 (medium/high, alerts #57 #58)
- @babel/core transitive -> ^7.29.6 via override (low, alert #56)
- @angular/common, @angular/core, @angular/compiler 19.x -> 20.3.25 (high/medium, alerts #52 #53 #54 #55)

Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>

* chore: align @openfeature/web-sdk with angular-sdk peer dependency

Address Copilot review feedback on PR #253: @openfeature/angular-sdk
1.3.1 requires @openfeature/web-sdk ^1.9.0 as a peer dependency, but
package.json still allowed ^1.7.3. Bump the declared range to match
and regenerate the lockfile.

Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>

* chore: resolve additional dependabot alerts for fast-uri

- fast-uri ^3.1.2 -> ^3.1.4 (high, alert #59: host confusion via failed IDN canonicalization)
- fast-uri ^3.1.2 -> ^3.1.4 (high, alert #60: host confusion via literal backslash authority delimiter)

Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>

* fix(deps): regenerate OpenAPI client for oapi-codegen v2.8.0

Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>

* chore: resolve postcss alert and pin fast-uri exactly

- postcss <= 8.5.17 -> ^8.5.18 override (high, alert #62)
- pin fast-uri override to exact 3.1.4 per CodeRabbit review suggestion

Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>

* chore: bump fast-uri to 3.1.5 for dependabot alert #64

- fast-uri 3.1.4 -> 3.1.5 (high, alert #64: host confusion via backslash authority introducer)

Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>

* chore: resolve open dependabot security alerts

- @angular/core 20.3.25 -> 20.3.28 (high, alert #75: XSS via event-handler attributes)
- @angular/compiler 20.3.25 -> 20.3.28 (high, alert #74: XSS via event-handler attributes)
- @angular/common 20.3.25 -> 20.3.28 (high, alert #73: cache-key ambiguity in HttpTransferCache)
- brace-expansion 2.1.0 -> 2.1.4 (high, alert #72: DoS via exponential-time expansion)

Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>

---------

Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants