-
Notifications
You must be signed in to change notification settings - Fork 271
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
Add domains and subnets as puppet resources that interact with the Foreman API #1047
base: master
Are you sure you want to change the base?
Conversation
a2aeacc
to
922ce67
Compare
922ce67
to
23d4058
Compare
23d4058
to
d7b86b2
Compare
Looks like CI didn't ran. Could you rebase? |
@rjemanuele I'd still be interested in getting this in. Could you rebase this? |
e4b539e
to
bc173c6
Compare
Rebased :) |
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.
Some minor comments, but otherwise I think this looks good.
if !value.nil? and value.start_with?("search=") then | ||
lookup_uri = "api/v2/" + id_name + "?" + value | ||
lookup = request(:get, lookup_uri) |
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.
I think this creates some redundancy. We already have request
which accepts a params
hash so if anything this should pass search as a hash. That properly escapes it.
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.
@ekohl is there an example of using request
I could have a look at to test with and make sure this works with it?
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.
There are some examples of using #request in its spec coverage: https://github.com/theforeman/puppet-foreman/blob/master/spec/unit/foreman_resource_rest_v3_spec.rb#L64-L131
desc 'foreman_domain creates a domain in foreman.' | ||
|
||
instance_eval(&PuppetX::Foreman::Common::REST_API_COMMON_PARAMS) | ||
instance_eval(&PuppetX::Foreman::Common::FOREMAN_DOMAIN_PARAMS) |
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.
If you're not reusing these (and it looks like you're not), it's better to inline them.
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.
Can you help me out here with what you're looking for? I'm more of a C, C++, Python programmer, very new to Ruby.
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 request is to move the methods being generated by PuppetX::Foreman::Common::FOREMAN_DOMAIN_PARAMS
directly into this file on the foreman_domain
type instead of the extra layer of meta-programming since there isn't another type which will reuse these methods.
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.
Stated another way, the methods in lib/puppet_x/foreman/common.rb
should be moved into foreman_*/rest_v3.rb
.
desc 'foreman_subnet creates a subnet in foreman.' | ||
|
||
instance_eval(&PuppetX::Foreman::Common::REST_API_COMMON_PARAMS) | ||
instance_eval(&PuppetX::Foreman::Common::FOREMAN_SUBNET_PARAMS) |
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.
This can also be inlined, like domain.
@rjemanuele Are you planning to take this over the finish line? I'm interested in these types as well. |
@jhoblitt Sure, I'll get this cleaned up and ready in the next week. |
@rjemanuele Great. Let me know if plans change and I'll try to pickup the ball. |
@rjemanuele Are you still stuck? |
@rjemanuele ping |
This will add support for creating domains and subnets within Foreman using Foreman's API from Puppet.
This also adds the ability to lookup IDs for some values with a "search=" string.