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 89.47368% with 4 lines in your changes missing coverage. Please review.

Project coverage is 64.25%. Comparing base (140b023) to head (d84f66d).

Files	Patch %	Lines
internal/ext/importer.go	71.42%	1 Missing and 1 partial ⚠️
internal/storage/sql/common/flag.go	92.30%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3196      +/-   ##
==========================================
+ Coverage   64.19%   64.25%   +0.06%     
==========================================
  Files         168      168              
  Lines       13559    13582      +23     
==========================================
+ Hits         8704     8727      +23     
  Misses       4186     4186              
  Partials      669      669              

Flag	Coverage Δ
unittests	64.25% <89.47%> (+0.06%)	⬆️

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

[markphelps]

looking great! commented on failing ci tests

[markphelps]

Looks great! Thank you both @kvnhmn and @erka !

[markphelps]

@all-contributors please add @kvnhmn for code

[allcontributors]

@markphelps

I've put up a pull request to add @kvnhmn! 🎉