Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Feature test for get-config hook and 'juju config' output. #7132
Conversation
jameinel
approved these changes
Mar 21, 2017
Have you tried running this without your original fixes? Would it have caught the discrepancy between the two implementations?
If you can put it into something like the juju 2.0 codebase and see that it fails, then I'm quite happy with it.
I did have one question about how we're handling nil values, especially WRT get-config hook.
| +func (s *ApplicationConfigSuite) assertHookOutput(c *gc.C, obtained charm.Settings, expected settingsMap) { | ||
| + c.Assert(len(obtained), gc.Equals, len(expected)) | ||
| + c.Assert(len(obtained), gc.Equals, len(s.settingKeys)) | ||
| + for name, aSetting := range expected { |
jameinel
Mar 21, 2017
Owner
I would have thought for 'expected' values which are 'nil', that the hook output would just omit them, rather than transmitting 'nil' as a value. Or is that part of what you changed?
eg, I would have thought there would be a pass where 'expected' has 'nil' values filtered out.
anastasiamac
Mar 21, 2017
Member
nils are filtered out on the final output at the jujuc/command unless the user supplies --all flag. Then all settings including the ones with nil values are returned.
| + "intnodefault": {Default: true}, | ||
| + "intoverwrite": {1620, false}, | ||
| + "strdefault": {"charm default", true}, | ||
| + "strnodefault": {Default: true}, |
jameinel
Mar 21, 2017
Owner
So I see that you have asserts like:
c.Assert(aSetting.Value, gc.Equals, obtained[name].Value)
But here you aren't defining a Value. What do we actually get out of this, then? I suppose with nothing set then "Value" is just 'nil'. Would it be more obvious to just set it directly to:
{nil, true}
instead of using the Zero value to do that? I feel like it would make the test setup a bit easier to parse, since it would be consistent.
anastasiamac
added some commits
Mar 22, 2017
|
@jameinel While testing without the original fix, I've discovered the urge to also add setting of nil values on deploy to catch the scenario that started it all \o/ I am ready to merge this PR. |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Generating tarball failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
anastasiamac commentedMar 21, 2017
•
Edited 1 time
-
anastasiamac
Mar 21, 2017
Description of change
Get-config hook and 'juju config' share similar logic but the code cannot be refactored into a common method. This PR introduces a feature test to ensure that we obtain the same values and defaults from both commands.
In addition, it was discovered that the logic that was determining whether a setting value was charm default was not working well for situations where user specified value that was the same as default or when default value was nil and user has not specified value either. This PR fixes this case.
QA steps
Unit tests pass.
Documentation changes
n/a
Bug reference
n/a