This repository has been archived by the owner on Nov 8, 2022. It is now read-only.
Fixes #1222 and #1228 : Applies config defaults - Metric too ambiguous #1225
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jcooklin
force-pushed
the
ib/1222-default-config-values
branch
3 times, most recently
from
37d6f8e to
ae07fbe
Compare
jcooklin
changed the title
|
The second commit starts |
jcooklin
force-pushed
the
ib/1222-default-config-values
branch
from
ae07fbe to
04d873c
Compare
|
Updated commit message. |
jcooklin
force-pushed
the
ib/1222-default-config-values
branch
2 times, most recently
from
b1934be to
ef006fa
Compare
|
LGTM |
IRCody
reviewed
Oct 3, 2016
| @@ -815,9 +815,6 @@ func TestMetricConfig(t *testing.T) { | |||
| errs := c.subscriptionGroups.validateMetric(m1) | |||
| So(errs, ShouldBeNil) | |||
| }) | |||
| Convey("So mock should have name: bob config from defaults", func() { | |||
| So(c.Config.Plugins.pluginCache["0"+core.Separator+"mock"+core.Separator+"1"].Table()["name"], ShouldResemble, ctypes.ConfigValueStr{Value: "bob"}) | |||
| }) | |||
There was a problem hiding this comment.
If this is still applying defaults, why was this test no longer valid?
There was a problem hiding this comment.
It wasn't valid here since the subscription group would need to be processed before the default would be applied to the plugin config cache. I added the test back as part of the convey that runs subscriptionGroups.ValidateMetric.
| if plugin.TypeName() != core.CollectorPluginType.String() { | ||
| plugins = append(plugins, plugin) | ||
| // add defaults to plugins (exposed in a plugins ConfigPolicy) | ||
| if lp, err := s.pluginManager.get( |
There was a problem hiding this comment.
Would this be cleaner as:
key := fmt.Sprintf("%s:%s:%d",plugin.TypeName(), plugin.Name(), plugin.Version())
lp, err := s.pluginManager.get(key)
if err == nil && lp.ConfigPolicy != nil {
...There was a problem hiding this comment.
I don't think so however if you feel strongly I'll change it.
There was a problem hiding this comment.
I don't feel super strongly.
| plugin.TypeName(), | ||
| plugin.Name(), | ||
| plugin.Version())); err == nil && lp.ConfigPolicy != nil { | ||
| if policy := lp.ConfigPolicy.Get([]string{""}); policy != nil && len(policy.Defaults()) > 0 { |
There was a problem hiding this comment.
Similar comment on this line.
* Adds ApplyDefaults(map[string]ctypes.ConfigValue) to ConfigDataNode * Renames ReverseMerge to ReverseMergeInPlace on ConfigDataNode * Adds ReverseMerge to ConfigDataNode returning a copy of the merged node ReverseMerge, which now returns a copy, is called when validating deps (task creation) and when loading plugins.
jcooklin
force-pushed
the
ib/1222-default-config-values
branch
from
2d560d0 to
ef91723
Compare
IRCody
reviewed
Oct 7, 2016
| @@ -1391,7 +1394,7 @@ func TestCollectSpecifiedDynamicMetrics(t *testing.T) { | |||
| So(err, ShouldBeNil) | |||
| // metric catalog should contain the 3 following metrics: | |||
| // /intel/mock/foo; /intel/mock/bar; /intel/mock/*/baz | |||
| So(len(mts), ShouldEqual, 3) | |||
| So(len(mts), ShouldEqual, 4) | |||
There was a problem hiding this comment.
Should we add the /intel/mock/all/baz to the comment so it's accurate?
| if plugin.TypeName() != core.CollectorPluginType.String() { | ||
| plugins = append(plugins, plugin) | ||
| // add defaults to plugins (exposed in a plugins ConfigPolicy) | ||
| if lp, err := s.pluginManager.get( |
There was a problem hiding this comment.
I don't feel super strongly.
* Updates validateMetric on subscriptionGroups to use GetMetrics instead of GetMetric since some namespace requests can expand to include multiple metrics.
jcooklin
force-pushed
the
ib/1222-default-config-values
branch
from
ef91723 to
92e2486
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1222 and #1228
Summary of changes for #1222 (applies config defaults):
ReverseMergeon ConfigDataNode toReverseMergeInPlaceReverseMergeon ConfigDataNode which returns a new ConfigDataNode without modifying the sourceApplyDefaultson ConfigDataNodeTesting done:
Example: After a task is started the configuration for which file will be published to is updated through the API. The next time the task fires this configuration is applied.
Summary of changes for #1228
Testing done:
Below is a screen capture that successful runs the tasks described in #1228.
Note: These two issues were included in the same PR since #1228 depended on #1222. #1228 adds test coverage across
ValidateDepsand as a result needed to apply defaults from the mocks config policy.@intelsdi-x/snap-maintainers