AddNew<type>Rule() as default way to create a rule #31

kindermoumoute · 2016-09-28T21:20:23Z

This commit changes 3 things that are linked:

Fixes issue that was overwriting a same type rule when it was using a
common key.
Change ConfigPolicy structure. Fields are now private and use
same types than rpc ConfigPolicy.
Mix New<typeRule() and Add<type>Rule() in one function:
AddNew<type>Rule().

kindermoumoute · 2016-09-28T21:22:53Z

examples/collector/rand/rand_test.go


 		Convey("No error returned", func() {
 			So(err, ShouldBeNil)
 		})
-
-		Convey("Has interger rule", func() {


This kind of test is already done in the library tests. Do we still want to make this possible?

yes, we do. It's for the unit test of rand.go

candysmurf

Overall, it looks fine. But I wish result check is not hard coded but as before in the expected results. The benefit is easy to change values if I like to test different scenarios or different values. Now I have to go into each line of code to change that.

candysmurf · 2016-09-29T17:13:04Z

v1/plugin/string_test.go

 				} else {
-					sr, err = NewStringRule(c.input.key, c.input.req)
+					err = p.AddNewStringRule(c.input.ns, c.input.key, c.input.req)


removing expected result comparison?

Added expected result in config_policy_test.go.

candysmurf · 2016-09-29T17:13:53Z

v1/plugin/string_test.go

 }

 func stringPassTestCases() []testCaseString {
 	tc := []testCaseString{
 		testCaseString{
-			expected: stringRule{Key: "xyz", Required: true},
-			input:    stringInput{key: "xyz", req: true},
+			input: stringInput{key: "xyz", req: true},


Shouldn't keep expected here?

Moved tests with expected result to config_policy_test.go.

candysmurf · 2016-09-29T19:56:48Z

v1/plugin/config_policy_test.go

-								So(vv.Required, ShouldEqual, srule.Required)
-								So(vv.HasDefault, ShouldEqual, srule.HasDefault)
-								So(vv.Default, ShouldEqual, srule.Default)
+								if kk == c.output.key {


keys should be always equal. Why do you need if statement here?

v.GetRules() still return previous test cases of the same type. In this PR there are 2 string test cases, and 2 bool test cases. So we don't want to test the current rule with a former one.

But we still verify that the rule is there with the exist variable.

IRCody · 2016-09-30T04:01:43Z

v1/plugin/bool.go


 // SetDefaultBool Allows easy setting of the Default value for an boolRule.
 // Usage:
 //		NewBoolRule(key, req, config.SetDefaultBool(default))
 func SetDefaultBool(in bool) boolRuleOpt {
-	return func(i *boolRule) {
+	return func(i *rpc.BoolRule) {


I'm curious what you think about potentially merging the type files into one since they no longer contain structs.

IRCody · 2016-09-30T04:06:41Z

v1/plugin/bool_test.go

 				} else {
-					br, err = NewBoolRule(c.input.key, c.input.req)
+					err = p.AddNewBoolRule(c.input.ns, c.input.key, c.input.req)


Is this testing the same thing? Wouldn't we want to check that the rule that is created is the same was what was expected to keep parity?

Same comment than above #31 (comment)

Those tests have been moved to config_policy_test.go.

IRCody · 2016-09-30T04:07:09Z

v1/plugin/bool_test.go

-			expected: boolRule{},
-			input:    input{key: "", req: false, opts: SetDefaultBool(false)},
+		testCaseBool{
+			input: boolInput{key: "", req: false, opts: SetDefaultBool(false)},


Line width.

IRCody · 2016-09-30T04:08:09Z

v1/plugin/config_policy.go

+// and optionally:
+//		config.SetDefaultInt(int64),
+//		config.SetMinInt(int64),
+//		config.SetMaxInt(int64),


I think the comment on the NewIntRule was already wrong, but this is no longer in the config package. Could you update the examples to plugin.Set()? Applies to the others also.

IRCody · 2016-09-30T04:09:57Z

v1/plugin/config_policy.go

-		ret.BoolPolicy[k].Rules[v.Key] = r
-	}
-
-	return ret


IRCody · 2016-09-30T04:20:14Z

v1/plugin/integer.go

-}
-
-type integerRuleOpt func(*integerRule)
+type integerRuleOpt func(*rpc.IntegerRule)

 // SetDefaultInt Allows easy setting of the Default value for an integerRule.
 // Usage:
 //	//	NewIntegerRule(key, req, config.SetDefaultInt(default))


Need to modify these comments that reference functions that don't exists anymore.

This commit changes 3 things that are linked: - Fixes issue that was overwriting a same type rule when it was using a common key. - Change ConfigPolicy structure. Fields are now private and use same types than rpc ConfigPolicy. - Mix `New<typeRule()` and `Add<type>Rule()` in one function: `AddNew<type>Rule()`.

This concern only 3 plugins that use the new Go library. It's an update after this PR got merged intelsdi-x/snap-plugin-lib-go#31

kindermoumoute force-pushed the fix_glitch_and_new_feature branch from ed550f7 to 00b7922 Compare September 28, 2016 22:26

kindermoumoute commented Sep 28, 2016

View reviewed changes

candysmurf suggested changes Sep 29, 2016

View reviewed changes

kindermoumoute force-pushed the fix_glitch_and_new_feature branch from 00b7922 to 43c3003 Compare September 29, 2016 18:22

candysmurf approved these changes Sep 29, 2016

View reviewed changes

candysmurf reviewed Sep 29, 2016

View reviewed changes

IRCody suggested changes Sep 30, 2016

View reviewed changes

kindermoumoute force-pushed the fix_glitch_and_new_feature branch from 43c3003 to a9366c7 Compare September 30, 2016 16:33

IRCody approved these changes Sep 30, 2016

View reviewed changes

IRCody merged commit d7be151 into intelsdi-x:master Sep 30, 2016

kindermoumoute added a commit to kindermoumoute/snap that referenced this pull request Sep 30, 2016

Update config policy rules in plugin examples

5908fc7

This concern only 3 plugins that use the new Go library. It's an update after this PR got merged intelsdi-x/snap-plugin-lib-go#31

kindermoumoute mentioned this pull request Sep 30, 2016

Update config policy rules in plugin examples intelsdi-x/snap#1247

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AddNew<type>Rule() as default way to create a rule #31

AddNew<type>Rule() as default way to create a rule #31

kindermoumoute commented Sep 28, 2016

kindermoumoute Sep 28, 2016

candysmurf Sep 29, 2016

candysmurf left a comment

candysmurf Sep 29, 2016

kindermoumoute Sep 29, 2016 •

edited

Loading

candysmurf Sep 29, 2016

kindermoumoute Sep 29, 2016

candysmurf Sep 29, 2016

kindermoumoute Sep 29, 2016

IRCody Sep 30, 2016

IRCody Sep 30, 2016

kindermoumoute Sep 30, 2016

IRCody Sep 30, 2016

IRCody Sep 30, 2016 •

edited

Loading

IRCody Sep 30, 2016

IRCody Sep 30, 2016

AddNew<type>Rule() as default way to create a rule #31

AddNew<type>Rule() as default way to create a rule #31

Conversation

kindermoumoute commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candysmurf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kindermoumoute Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IRCody Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kindermoumoute Sep 29, 2016 •

edited

Loading

IRCody Sep 30, 2016 •

edited

Loading