Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flow to create simple boolean flag #354

Merged
merged 2 commits into from
Apr 10, 2020
Merged

Add flow to create simple boolean flag #354

merged 2 commits into from
Apr 10, 2020

Conversation

dcramer
Copy link
Contributor

@dcramer dcramer commented Apr 9, 2020

This exposes a way to create a new pre-filled simple flag. It includes a single segment, variant, and predefined distribution allowing you to more easily create a yes/no feature flag check.

It's exposed using the {template: "simple"} parameter on the flag creation endpoint. Additionally it exposes the action in the UI as a dropdown alongside the existing Create Flag action.

image

Fixes #345

@dcramer
Copy link
Contributor Author

dcramer commented Apr 9, 2020

Feel free to nitpick the tests/go/etc here as I just threw stuff together. Haven't ever touched swagger or Vue, and haven't written Go in years.

Also if there are more descriptive/obvious names we can use for 'template' or 'simple' its worth thinking about that.

@@ -21,18 +21,21 @@ github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRF
github.com/apache/thrift v0.12.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb+bacwQ=
github.com/asaskevich/govalidator v0.0.0-20180315120708-ccb8e960c48f h1:y2hSFdXeA1y5z5f0vfNO0Dg5qVY036qzlz3Pds0B92o=
github.com/asaskevich/govalidator v0.0.0-20180315120708-ccb8e960c48f/go.mod h1:lB+ZfQJz7igIIfQNfa7Ml4HSf2uFQQRzpGGRXenZAgY=
github.com/auth0/go-jwt-middleware v0.0.0-20170425171159-5493cabe49f7 h1:irR1cO6eek3n5uquIVaRAsQmZnlsfPuHNz31cXo4eyk=
Copy link
Contributor Author

Choose a reason for hiding this comment

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

im guessing these are all ok, but every time I run builds it re-adds them to go.sum, so maybe worth just sorting out/committing

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, usually they are ok, especially when it passes the unit/integration tests.

<el-input
placeholder="Specific new flag description"
v-model="newFlag.description">
<el-input placeholder="Specific new flag description" v-model="newFlag.description">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lots of changes in here from prettier, but most of this is just the onCommandDropdown handler and related elements

return flag.NewCreateFlagDefault(500).WithPayload(
ErrorMessage("cannot create flag. %s", err))
}

if params.Body.Template == "simple" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping there might be a better way to use a constant here, but got a bit overwhelmed when going down that route and figuring out how it'd make sense w/ swagger/etc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, I think swagger is more for schema, inside backend, we can use constants as validation. Actually, it's also a good opportunity to do some refactoring, this function is too long. We can have a file crud_flag_creation.go in the same handler folder, and dedicates the creation by template logic there. Also the template name simple, variant on key can be defined as constants.

For the naming of template, how about we start with something explicit first simple_boolean_flag instead of simple? It's easier to extend that to simple_ab_experiment and simple_dynamic_configuration in the future.

@@ -142,6 +186,7 @@ func (c *crud) CreateFlag(params flag.CreateFlagParams) middleware.Responder {
resp.SetPayload(payload)

entity.SaveFlagSnapshot(getDB(), f.ID, getSubjectFromRequest(params.HTTPRequest))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should this be part of the transaction somehow? just mimicking other endpoints right now but maybe something for followup

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, tracked here #355

},
})
assert.NotZero(t, res.(*flag.CreateFlagOK).Payload.ID)
assert.Equal(t, "simple_flag_key", res.(*flag.CreateFlagOK).Payload.Key)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could assert more on the return payload here, as I realized it should return variants/segments as part of it meaning we can do full validation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually looks like payload isnt correct - it does save segment/variant/etc but doesnt return in payload.. will explore but if you have guidance lmk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed this but would appreciate feedback if the way im managing the arrays is correct

assert.Equal(t, segment.Rank, entity.SegmentDefaultRank)

variant := entity.Variant{FlagID: flagID}
db.First(&variant)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

would have preferred to do .Find() and then assert, but couldnt actually figure out how to construct the query/objects. also didnt read any kind of docs so maybe its not that hard ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, First works, since you are finding the first variant with the flagID you just created. Some syntax for query. https://gorm.io/docs/query.html

@dcramer
Copy link
Contributor Author

dcramer commented Apr 9, 2020

question for anyone paying attention: are there other kinds of templates that could exist? is that kind of behavior something that might be valuable in heavy use of this system?


if err := tx.Create(s).Error; err != nil {
tx.Rollback()
return segment.NewCreateSegmentDefault(500).WithPayload(ErrorMessage("%s", err))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reading this i think these actually need to be all NewCreateFlagDefault

@codecov-io
Copy link

Codecov Report

Merging #354 into master will decrease coverage by 0.41%.
The diff coverage is 62.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #354      +/-   ##
==========================================
- Coverage   81.75%   81.33%   -0.42%     
==========================================
  Files          26       26              
  Lines        1540     1570      +30     
==========================================
+ Hits         1259     1277      +18     
- Misses        211      219       +8     
- Partials       70       74       +4     
Impacted Files Coverage Δ
pkg/handler/crud.go 87.33% <62.50%> (-2.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c703eb6...848bcf0. Read the comment docs.

Copy link
Collaborator

@zhouzhuojie zhouzhuojie left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, awesome work! Left a few comments about refactoring.

@@ -21,18 +21,21 @@ github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRF
github.com/apache/thrift v0.12.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb+bacwQ=
github.com/asaskevich/govalidator v0.0.0-20180315120708-ccb8e960c48f h1:y2hSFdXeA1y5z5f0vfNO0Dg5qVY036qzlz3Pds0B92o=
github.com/asaskevich/govalidator v0.0.0-20180315120708-ccb8e960c48f/go.mod h1:lB+ZfQJz7igIIfQNfa7Ml4HSf2uFQQRzpGGRXenZAgY=
github.com/auth0/go-jwt-middleware v0.0.0-20170425171159-5493cabe49f7 h1:irR1cO6eek3n5uquIVaRAsQmZnlsfPuHNz31cXo4eyk=
Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, usually they are ok, especially when it passes the unit/integration tests.

docs/api_docs/bundle.yaml Show resolved Hide resolved
return flag.NewCreateFlagDefault(500).WithPayload(
ErrorMessage("cannot create flag. %s", err))
}

if params.Body.Template == "simple" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, I think swagger is more for schema, inside backend, we can use constants as validation. Actually, it's also a good opportunity to do some refactoring, this function is too long. We can have a file crud_flag_creation.go in the same handler folder, and dedicates the creation by template logic there. Also the template name simple, variant on key can be defined as constants.

For the naming of template, how about we start with something explicit first simple_boolean_flag instead of simple? It's easier to extend that to simple_ab_experiment and simple_dynamic_configuration in the future.

@@ -142,6 +186,7 @@ func (c *crud) CreateFlag(params flag.CreateFlagParams) middleware.Responder {
resp.SetPayload(payload)

entity.SaveFlagSnapshot(getDB(), f.ID, getSubjectFromRequest(params.HTTPRequest))
Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, tracked here #355

assert.Equal(t, segment.Rank, entity.SegmentDefaultRank)

variant := entity.Variant{FlagID: flagID}
db.First(&variant)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, First works, since you are finding the first variant with the flagID you just created. Some syntax for query. https://gorm.io/docs/query.html

@dcramer
Copy link
Contributor Author

dcramer commented Apr 10, 2020

Thanks! Updated based on feedback.

@zhouzhuojie zhouzhuojie merged commit fd702e9 into openflagr:master Apr 10, 2020
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.

[feat] Support variant-less rollouts
3 participants