open-feature / go-sdk Goto Github PK

Change in the documentation

Whilst it's common to call the language "golang", the official name of the language is "go" (https://en.wikipedia.org/wiki/Go_(programming_language)

Not that it mases a lot of difference, but should we consider renaming the repo (and associated docs) from golang to go?

In previous versions of the Golang-sdk EvaluationContext has been defined as an interface{}. In this commit it has been updated to a struct containing required fields (currently just TargetingKey but likely to grow) with a nested map[string]interface{} under the Attributes field, to better comply to the spec.

I believe this may cause issues down the line for client applications needing to know if their properties are on the base struct or in the additional properties.

if evCtx.Yellow == true {
    return 'yellow'
}

turns into

if evCtx.Attributes.Yellow == true {
    return 'yellow'
}

Following conversations with @skyerus we have identified 3 possible ways to resolve this, though this list likely isn't exhaustive:

1. Do nothing and handle the nesting within providers
2. Flatten the structure into a map[string]interface{} as standard and update the provider interface method signatures to reflect this
3. Offer a before hook that will flatten the structure (would require provider hooks), similar outcome to doing nothing, but the hook would be directly linked to the sdk

Please let me know your thoughts

EDIT by @toddbaert :

Seems like we have a consensus, I will take this issue and action it.

Requirements

While other breaking changes are taking place (#56 & #57) it's a good time to review whether any quality of life improvements can be made to application author facing APIs.

Apologies if this has already been discussed.

I may be missing something, but is it possible to add Go's context.Context as a first parameter to all client/provider methods (interface definitions)?

Ex:

would become something like:

BooleanValue(ctx context.Context, flag string, defaultValue bool, evalCtx EvaluationContext, options EvaluationOptions) (bool, error)

It seems as a user of this SDK that I would want to be able to use context to pass request timeouts upstream to the provider (ie: in case request to provider is taking too long to return, error which would return the 'default' value).

Is this meant to be done via a Hook per request? If so would it be possible to document this usage?

Although it still seems that passing context.Context as a first class (first) parameter would be ideal IMO since it is a Go convention.

Requirements

Currently, anyone who wants to create a new hook is forced to implement all of the hook interface

type Hook interface {
	Before(hookContext HookContext, hookHints HookHints) (*EvaluationContext, error)
	After(hookContext HookContext, flagEvaluationDetails EvaluationDetails, hookHints HookHints) error
	Error(hookContext HookContext, err error, hookHints HookHints)
	Finally(hookContext HookContext, hookHints HookHints)
}

This is cumbersome, an alternative is to split this into an interface for each func and assert the given hook to each interface.
This makes application author's lives easier at the cost of marginally more complexity in the sdk and some type assertions.

I think this is a worthwhile trade, thoughts? @beeme1mr @toddbaert @james-milligan

Change in the documentation

Having CODEOWNERS is a repository requirement.

Requirement 1.6.1

The client SHOULD transform the evaluation context using the provider's context transformer function if one is defined, before passing the result of the transformation to the provider's flag resolution functions.

Replace this todo with the test.

Requirements

By default the leveled logger is only outputting errors, via log.Println(err), which in turn calls the ProviderResolutionDetail.Error().

This combination means you end up with logs that look like 2022/10/07 09:09:08 GENERAL because only the error code is returned by Error().

My preference would be to not log anything like the other levels do; Error(...) would be noop, leaving this to userland to decide what they want logging using SetLogger. If not, perhaps the message should include more details & make it clear(er) where it originates from, as when interleaved with other logs it makes hunting the cause down tricky.

Wanted to gather any thoughts & opinions first, but I'd be happy to attempt a PR depending on the outcome of this issue. Felt like a sharp edge and we could improve the out-of-the-box experience.

Observed behavior

I am using nil as a default value while calling client.ObjectValueDetails but when doing this everytime if the default value is used we receive no ResolutionDetail from the SDK.

After looking a bit at the SDK it seems that we add the ResolutionDetail only if the value is not nil.

It means that we cannot use nil as value in the SDK.

Expected Behavior

I would like to be able to use nil as value for my flag.

Steps to reproduce

SDK return a flag with a nil value.

Requirements

• Create a GitHub release automatically based on a git tag
• The git tag should match v*.*.* and follow semver guidelines.
• Breaking changes should be represented in PRs and Release notes
• Breaking changes should NOT bump major version numbers until a 1.0 release
• #89

Overview

Golang 1.18 introduces generics, which would be useful for the Golang OpenFeature SDK. However, since1.18 is only a few months old we need to way the pros and cons of switching.

Alpha Checklist:

• spec compliant
• contains test suite which verifies behavior consistent with spec
• #10
• #11
• comprehensive readme

Change in the documentation

See https://pkg.go.dev/search?q=openfeature&m=package vs https://pkg.go.dev/search?q=opentelemetry

The CNCF require that we start the process of achieving a OpenSSF best practices badge.

https://bestpractices.coreinfrastructure.org/en

The tool scans a repo and provides a report of the items that are incomplete or missing.

Currently the sdk exposes NumberValue/ NumberValueDetails functions which return float64, there is no equivalent for int64. The spec states that there should be functions for both.

Amend NumberValue to be FloatValue and create IntValue / IntValueDetails functions.

Implement a code coverage check in the github workflow. After tests are run, coverage should be reported. The check may or may not reject PRs which drop coverage by a certain threshold.

See open-feature/js-sdk#64.

Blocked by: #13

The FeatureProvider interface uses a map[string]interface{} instead of a EvaluationContext. Was this done intentionally?

This question was asked by @rgrassian-split on Slack.

This issue lists Renovate updates and detected dependencies. Read the Dependency Dashboard docs to learn more.

Open

These updates have all been created already. Click a checkbox below to force a retry/rebase of any.

• fix(deps): update golang.org/x/exp digest to 814bf88
• chore(deps): update actions/setup-go action to v5
• chore(deps): update codecov/codecov-action action to v4
• chore(deps): update cyclonedx/gh-gomod-generate-sbom action to v2
• chore(deps): update google-github-actions/release-please-action action to v4
• chore(deps): update goreleaser/goreleaser-action action to v5
• Click on this checkbox to rebase all open PRs at once

Detected dependencies

• Check this box to trigger a request for Renovate to run again on this repository

The evaluation details value type should correspond to the method called by the user. For example, if the user is calling the bool details method, the value should be a boolean.

The README.md needs to be updated to reflect the state of the sdk

Update the SDK to be compliant with the v0.5.1 spec.

What's Changed

• chore: add additional document statuses by @toddbaert in open-feature/spec#139
• chore: add status badges by @toddbaert in open-feature/spec#148
• chore: section clarity, harden API, providers by @toddbaert in open-feature/spec#149

Update the badge on the readme to

Full Changelog: open-feature/spec@v0.5.0...v0.5.1

Requirements

Define a logger interface that conforms to Go's most prevalent logging idioms (look at most popular libraries).

Allow an application author to set the sdk's logger at global level (and client level?).

Add logging at all levels in the sdk (debug, info, warn, error).

cc @beeme1mr @toddbaert

Issue templates should be added. They can be modified versions of the ones used in FlagD.

https://github.com/open-feature/flagd/tree/main/.github/ISSUE_TEMPLATE

Update the SDK to be compliant with the v0.4.0 spec.

Providers

⚠ BREAKING CHANGES

• #129 - Remove EvaluationOptions from FeatureProvider method signatures

General

• Add/update a spec version badge on the readme.

Full Changelog: open-feature/spec@v0.3.1...v0.4.0

Requirements

A test suite for caching has been added to the test-harness repo, implement the tests.

Spec requirements: https://github.com/open-feature/spec/blob/main/specification/flag-evaluation.md

An example of an implemented spec requirement that requires a test:

Requirement 1.1.2

The API MUST provide a function to set the global provider singleton, which accepts an API-conformant provider implementation.

The following requirements can be tested:

Requirement 2.1
Requirement 2.2
Condition 2.3

Requirements

Fuzzing is a repository requirement.

Go supports fuzzing natively since 1.18, go-sdk is still using version 1.17. Either upgrade to 1.18 or use a third-party library.

Observed behavior

Looking at https://github.com/open-feature/golang-sdk/blob/main/pkg/openfeature/provider.go#L65-L68 and running the code, it is possible for a provider to return a struct for BoolResolutionDetail where there are two different Value elements, with different values.

eg

It's been a while since I've done go coding, and this may be idiomatic, but to me it's confusing

Expected Behavior

You should only be able to set one Value value

Steps to reproduce

	thing := openfeature.BoolResolutionDetail{
		Value: true,
		ResolutionDetail: openfeature.ResolutionDetail{
			Value: false,
		},
	}

Update the SDK to be compliant with the v0.2.0 spec.

Flag Evaluation API

• #116 - Have different methods for float and int

Providers

• #38

Evaluation Context

• #40
• #120 - Add the ability to fetch all custom fields
• #121 - Ensure evaluation context keys are unique

Hooks

• #119 - Ensure provider hook are correctly ordered

Full Changelog: open-feature/spec@v0.1.0...v0.2.0

Requirements

Inspired by open-feature/dotnet-sdk#56

The global singletons (e.g. provider, evaluationContext, logger) all share a mutex to ensure thread safety, however a client doesn't use a mutex so there's a concern if two processes want to set an evaluation context for example.

The EvaluationContext type has the field Attributes of type map[string]interface{}. In go, maps are always passed by reference, this means that in theory one could pass an EvaluationContext to a client's flag evaluation call and continue to alter the Attributes' map in another process.
This could be avoided by making the Attributes field unexported with an exported constructor for EvaluationContext, this would mean that an application author couldn't change the field once constructed. However, an author could still alter the map they passed as a parameter to the constructor, which is the same map used in struct (passed by reference). In order to avoid this the constructor would need to initialise a new map and copy the map passed as a parameter.

Is this overkill? Is there a better solution we can come up with?

cc @beeme1mr @toddbaert @james-milligan

Update the SDK to be compliant with the v0.3.0 spec.

Providers

• #129 - Remove context transformation

Hooks

• #125 - Verify context merging order for before hooks

Full Changelog: open-feature/spec@v0.2.0...v0.3.0

Observed behavior

Clients use old global logger after global logger has been changed

Expected Behavior

Clients to use updated global logger

Steps to reproduce

1. Create a client without explicitly setting the logger
2. Update global logger
3. Use client from step 1 and see that the old global logger is still used

As defined in the hooks spec.

Include test coverage where possible.

Change in the documentation

Add a link on the readme to https://pkg.go.dev/github.com/open-feature/go-sdk#section-readme

• Implement gherkin suite here
• run in CI
• See examples PRs in Java and JS SDKs

Requirements

Dependencies are not currently being autoupdated, this is required to conform with the repository requirements.

https://github.com/apps/renovate

We need automated publishing of the Golang SDK. Things to consider:

• versioning (alpha for now: 0.0.xxx)
• naming
• Anything else?

https://go.dev/doc/modules/publishing

Blocked by: #13

Update the SDK to be compliant with the v0.5.0 spec.

What's Changed

⚠ BREAKING CHANGES

• open-feature/spec#142 Error codes must be a constrained set of values (e.g. enum).

Additional changes

• open-feature/spec#140 Clarify that reason must be a string in the flag resolution details and evaluation details.
• open-feature/spec#142 Add optional error message to flag resolution details.

General

• Add/update a spec version badge on the readme.

Full Changelog: open-feature/spec@v0.4.0...v0.5.0

Add a test that ensures an error code is included in the evaluation details when the provider has an issue.

#67

Automate the detection of dependency vulnerabilities by utilising snyk as a code scanning tool in the pr-checks workflow.

Change in the documentation

Include a link to the list of providers and hooks available.

The current test suite names each test like so:

func TestRequirement_1_2_1(t *testing.T) {}

We'd like to change this to include a comment of the spec requirement:

// The client MUST provide a method to add hooks which accepts one or more API-conformant hooks, and appends them to the collection of any previously added hooks.
// When new hooks are added, previously added hooks are not removed.
func TestRequirement_1_2_1(t *testing.T) {}

Requirements

The existing gh actions are not generating SBOMs for the go-sdk module, this is required to conform with the repository requirements.

cycloneDX offers a github action which can be run as part of the release process for each module.

Currently there are getters defined that are prefixed with Get e.g.

GetBooleanValue(flag string, defaultValue bool, evalCtx EvaluationContext, options EvaluationOptions) (bool, error)

This goes against Golang convention. Renaming this example to just BooleanValue would be better practice.

We should add a workflow that does both testing and linting on each PR targeting main. See flagd for an example. We should probably be consistent with that repo since it's also golang: https://github.com/open-feature/flagd/blob/main/.github/workflows/build.yaml