-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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 a dynamic inventory plugin for Online #44720
Conversation
The test
|
04bf7e6
to
44dd5e5
Compare
"location", | ||
"boot_mode", | ||
"power", | ||
"boot_mode", |
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.
Two times boot_mode
a4dd99e
to
c5e0330
Compare
Nice to have features:
|
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.
Globally looks good to me. The only blocker is the line that reflects a change on #43715. I'd like us to have an agreement before pushing it.
@@ -0,0 +1,190 @@ | |||
# Copyright (c) 2017 Ansible Project |
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.
s/2017/2018
def _fetch_information(url, token): | ||
headers = { | ||
'Authorization': "Bearer %s" % token, | ||
'Content-type': 'application/json' |
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.
suggestion: Adds an User-Agent
header that identifies that its coming from Ansible. (I think we should do that also on the main modules). Not a blocker.
raise AnsibleError("Error while fetching %s: %s" % (url, to_native(e))) | ||
|
||
try: | ||
raw_data = to_text(response.read(), errors='surrogate_or_strict') |
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 seems to be related to #43715. Can we get a final agreement on the other PR first before pushing that here?
if not hostname: | ||
return | ||
|
||
self.inventory.add_host(host=hostname) |
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 see there is no grouping in this inventory. Is this intentional for the first patch? Fine to me but I'd like to raise that grouping based on os_name
of offer
allow greater flexibility when dealing with a fleet of machine, might be interesting to add.
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'm working on a grouping feature that could be easily extended to geographical location and private networking subnets.
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.
Done for location and offer
5f159fe
to
14d1ded
Compare
c73c511
to
71fc817
Compare
Done for RPN |
71fc817
to
43e0c9a
Compare
shipit |
shipit |
rebuild_merge |
SUMMARY
Add a dynamic inventory plugin for online
ISSUE TYPE
COMPONENT NAME
ANSIBLE VERSION