Implement flag metadata

[kvnhmn]

Re: #2682 (only Backend implementation, no UI changes)

[GeorgeMac]

Hey @kvnhmn 🙌

This is amazing, thank you for taking a run at this.
We would love to help to get this over the line.
The bulk of the PR looks great 🙇

One thing that would help us would be to leverage the protobuf struct type (instead of the string type).
Really, we would like our old attachment to leverage that too, but its tricky with backwards compat and protobuf.
I added a comment in the review that gives some examples of where we use it in the auth storage layer for metadata there.

Another thing that would help is to update the declarative format.
You can find the definition for Flag here:

flipt/internal/ext/common.go

Lines 15 to 24 in fee220d

	type Flag struct {
	Key string `yaml:"key,omitempty" json:"key,omitempty"`
	Name string `yaml:"name,omitempty" json:"name,omitempty"`
	Type string `yaml:"type,omitempty" json:"type,omitempty"`
	Description string `yaml:"description,omitempty" json:"description,omitempty"`
	Enabled bool `yaml:"enabled" json:"enabled"`
	Variants []*Variant `yaml:"variants,omitempty" json:"variants,omitempty"`
	Rules []*Rule `yaml:"rules,omitempty" json:"rules,omitempty"`
	Rollouts []*Rollout `yaml:"rollouts,omitempty" json:"rollouts,omitempty"`
	}

Having Metadata here will allow folks to write it in yaml when either using a declarative backend or importing/exporting via the API.
This will need updating also in import and export:

• import

  flipt/internal/ext/importer.go

  Lines 123 to 129 in fee220d

  	req := &flipt.CreateFlagRequest{
  	Key: f.Key,
  	Name: f.Name,
  	Description: f.Description,
  	Enabled: f.Enabled,
  	NamespaceKey: namespace,
  	}

• export

  flipt/internal/ext/exporter.go

  Lines 129 to 135 in fee220d

  	flag := &Flag{
  	Key: f.Key,
  	Name: f.Name,
  	Type: f.Type.String(),
  	Description: f.Description,
  	Enabled: f.Enabled,
  	}

Lastly, would be awesome to get at-least a happy path case in the integration tests.
But also, don't want to overload you. This could also be something we do in a subsequent PR.

Thanks for contributing 🙌

[codecov]

Codecov Report

Attention: Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.

Project coverage is 63.44%. Comparing base (5dabf15) to head (9d91425).
Report is 1 commits behind head on main.

Files	Patch %	Lines
internal/storage/sql/fields.go	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3196      +/-   ##
==========================================
+ Coverage   63.42%   63.44%   +0.02%     
==========================================
  Files         166      166              
  Lines       13355    13369      +14     
==========================================
+ Hits         8470     8482      +12     
- Misses       4229     4230       +1     
- Partials      656      657       +1     

Flag	Coverage Δ
unittests	63.44% <88.88%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[GeorgeMac]

Thanks for making these changes @kvnhmn . It is looking great.

Unsure if you can see the failures in CI, but thought I would shine a light on some of them.

One of our test suites is seeing a non-nil pointer to an empty metadata:

         	            	--- Expected
        	            	+++ Actual
        	            	@@ -16779,3 +16779,16 @@
        	            	  Type: (flipt.FlagType) 0,
        	            	- Metadata: (*structpb.Struct)(<nil>)
        	            	+ Metadata: (*structpb.Struct)({
        	            	+  state: (impl.MessageState) {
        	            	+   NoUnkeyedLiterals: (pragma.NoUnkeyedLiterals) {
        	            	+   },
        	            	+   DoNotCompare: (pragma.DoNotCompare) {
        	            	+   },
        	            	+   DoNotCopy: (pragma.DoNotCopy) {
        	            	+   },
        	            	+   atomicMessageInfo: (*impl.MessageInfo)(<nil>)
        	            	+  },
        	            	+  sizeCache: (int32) 0,
        	            	+  unknownFields: ([]uint8) <nil>,
        	            	+  Fields: (map[string]*structpb.Value) <nil>
        	            	+ })
        	            	 })

Which, I think is fine for the API to return this, but either the API needs to change or that test case needs to be updated to ensure it is asserting a non-nil empty struct.

The other thing I am noticing is that the AFTER keyword on ALTER appears to not be supported by all of the relational DB backends.
What is weird is that the SQLite docs says it does not support it, but also, it doesn't fail on migration. Whereas e.g. Postgres fails explicitly during the migrations:

 35  : [27.4s] | Error: running migrations: migration failed: syntax error at or near "AFTER" (column 44) in line 1: ALTER TABLE flags ADD COLUMN metadata JSON AFTER enabled;
35  : [27.4s] |  (details: ERROR: syntax error at or near "AFTER" (SQLSTATE 42601))

We might need to drop that from the relevant migrations. It seems like maybe MySQL supports it, but not postgres and potentially SQLite just ignores it (so no harm I guess).

[kvnhmn]

Thanks for making these changes @kvnhmn . It is looking great.

Unsure if you can see the failures in CI, but thought I would shine a light on some of them.

One of our test suites is seeing a non-nil pointer to an empty metadata:

         	            	--- Expected
        	            	+++ Actual
        	            	@@ -16779,3 +16779,16 @@
        	            	  Type: (flipt.FlagType) 0,
        	            	- Metadata: (*structpb.Struct)(<nil>)
        	            	+ Metadata: (*structpb.Struct)({
        	            	+  state: (impl.MessageState) {
        	            	+   NoUnkeyedLiterals: (pragma.NoUnkeyedLiterals) {
        	            	+   },
        	            	+   DoNotCompare: (pragma.DoNotCompare) {
        	            	+   },
        	            	+   DoNotCopy: (pragma.DoNotCopy) {
        	            	+   },
        	            	+   atomicMessageInfo: (*impl.MessageInfo)(<nil>)
        	            	+  },
        	            	+  sizeCache: (int32) 0,
        	            	+  unknownFields: ([]uint8) <nil>,
        	            	+  Fields: (map[string]*structpb.Value) <nil>
        	            	+ })
        	            	 })

Which, I think is fine for the API to return this, but either the API needs to change or that test case needs to be updated to ensure it is asserting a non-nil empty struct.

The other thing I am noticing is that the AFTER keyword on ALTER appears to not be supported by all of the relational DB backends. What is weird is that the SQLite docs says it does not support it, but also, it doesn't fail on migration. Whereas e.g. Postgres fails explicitly during the migrations:

 35  : [27.4s] | Error: running migrations: migration failed: syntax error at or near "AFTER" (column 44) in line 1: ALTER TABLE flags ADD COLUMN metadata JSON AFTER enabled;
35  : [27.4s] |  (details: ERROR: syntax error at or near "AFTER" (SQLSTATE 42601))

We might need to drop that from the relevant migrations. It seems like maybe MySQL supports it, but not postgres and potentially SQLite just ignores it (so no harm I guess).

Thanks for your tip @GerogeMac.

I have extended a non-nil empty struct in the integration tests. I also removed it from all migrations, except for the one for MySql, for sqlite I also removed it to be on the safe side, hope that's okay. Also i extended the DEVELOPMENT.md, cause i missed more informations regarding unit-/integration-tests

[GeorgeMac]

Im seeing a lot of errors in tests again because the API clients and so on are expecting a non-nil metadata.
I think, accepting nil is something we probably can handle. It might work if you set each of these metadata fields as optional.

We could try that and see if it makes the tests happy.

Suggested change

	google.protobuf.Struct metadata = 10;
	optional google.protobuf.Struct metadata = 10;