-
Notifications
You must be signed in to change notification settings - Fork 17
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
(PDK-1143) changes to work with composite namevars from simple provider #65
Conversation
Codecov Report
@@ Coverage Diff @@
## master #65 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 36 36
Lines 848 831 -17
=====================================
- Hits 848 831 -17
Continue to review full report at Codecov.
|
name: 'foo', | ||
name: { | ||
route: 'foo/bar', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the real should
would not have the name:
hash like this. Is this an artifact of above test data changes with the name hash inline?
Also, I do not understand why there is only one attribute in the name hash here...
@@ -222,11 +232,19 @@ | |||
base_xpath: '/some_xpath', | |||
} | |||
end | |||
let(:name) do | |||
{ | |||
name: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary nesting here?
@@ -49,6 +49,9 @@ | |||
interval: '5', | |||
count: '4', | |||
enable: true, | |||
name: { | |||
path: 'path monitor', | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would this be easier if name:
were a key one level up? Then the shared examples could deal with passing through this, or the plain name? See also below for follow-up problems this causes
This is not a full review, and I get the feeling that overall this is working. The additional testing complexity is making it hard for me to review. |
ddf1088
to
f56c5d6
Compare
PDK-1143 allows the resource_api to work nicely with composite providers, this PR is a refactor of the `path_monitor` and `static_routes` which work with composite namevars.
f56c5d6
to
a17cabc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can merge this after puppet-agent 6.0.1 has released with the new Resource API drop.
puppet 6.0.1 has been released with the necessary changes |
PDK-1143 allows the resource_api to work nicely with composite providers, this PR is a refactor of the
path_monitor
andstatic_routes
which work with composite namevars.