Skip to content

Add put and delete for variants#37

Merged
zhouzhuojie merged 1 commit into
masterfrom
zz/put-delete-variant
Oct 17, 2017
Merged

Add put and delete for variants#37
zhouzhuojie merged 1 commit into
masterfrom
zz/put-delete-variant

Conversation

@zhouzhuojie
Copy link
Copy Markdown
Collaborator

Put and delete for variants

Comment thread pkg/handler/validate.go Outdated
for _, s := range f.Segments {
for _, d := range s.Distributions {
if d.VariantID == util.SafeUint(params.VariantID) && d.Percent != uint(0) {
return NewError(400, "error deleting variant %v. distribution %v still has non-zero distribution %v", params.VariantID, d.ID, d.Percent)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Note, we cannot delete a variant if it's rolling out to some non-zero distribution

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oo ok, yea, i can put that check on the frontend too

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

actually, i don't think you can delete a variant at all if some distribution is using it

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yup, actually I need to remove all the distribution that has the references to it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ohh, so do you want it so when they delete a variant, it deletes all referenced distributions? or just not allow that until they delete the distribution?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

going to delete the referenced distribution if percent is 0, otherwise, return 400

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oo ok

Copy link
Copy Markdown
Contributor

@lucidrains lucidrains left a comment

Choose a reason for hiding this comment

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

🇩🇪

@zhouzhuojie zhouzhuojie force-pushed the zz/put-delete-variant branch from 2914bd0 to e349d4e Compare October 17, 2017 00:01
@zhouzhuojie zhouzhuojie merged commit 518700d into master Oct 17, 2017
@zhouzhuojie zhouzhuojie deleted the zz/put-delete-variant branch November 14, 2017 02:08
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.

2 participants