Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixes #26068 - Improve inventory structure #258

Merged
merged 1 commit into from Apr 16, 2019
Merged

Fixes #26068 - Improve inventory structure #258

merged 1 commit into from Apr 16, 2019

Conversation

xprazak2
Copy link
Contributor

No description provided.

@xprazak2 xprazak2 force-pushed the inventory branch 2 times, most recently from 847f87a to 26e10c0 Compare March 29, 2019 11:05
@ares ares added the 3.0 the inventory format will change label Mar 29, 2019
ares
ares requested changes Mar 29, 2019
View reviewed changes
Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works well, question regarding http method, let's discuss this also with a bigger group on Monday

app/controllers/foreman_ansible/api/v2/hostgroups_controller_extensions.rb Outdated
@@ -60,6 +60,16 @@ def assign_ansible_roles
find_resource
process_response @hostgroup.update(:ansible_roles => @ansible_roles)
end

api :POST, '/hostgroups/ansible_inventory',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be POST? feels more like reading values, so GET may be more appropriate, is that because of potentially long hostgroup_ids? Maybe we should also accept search parameter to avoid specifying too long arrays (especially on host controller extensions)

app/controllers/foreman_ansible/api/v2/hosts_controller_extensions.rb Outdated
@@ -62,6 +65,16 @@ def ansible_roles
def assign_ansible_roles
process_response @host.update(:ansible_roles => @ansible_roles)
end

api :POST, '/hosts/ansible_inventory',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GET vs POST

@ares ares mentioned this pull request Mar 29, 2019
Closed
@ares
Copy link
Member

ares commented Apr 1, 2019

Few comments after a bit broader discussions:

  • let's allow both GET and POST
  • let's move API request to separate PR, we need to test them with ansible inventory script
  • the foreman: shouldn't contain the whole ENC but just content of parameters, not puppet env or puppet classes
  • ansible variables should win over parameters

@xprazak2
Copy link
Contributor Author

xprazak2 commented Apr 1, 2019

  • I removed the API endpoints
  • foreman key contains only content of parameters
  • ansible variabales winning over host params

test-inv

@ares
Copy link
Member

ares commented Apr 16, 2019

Thanks @xprazak2, merging!

@ares ares merged commit 72875cd into theforeman:master Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ares ares ares approved these changes

Assignees
No one assigned
Labels
3.0 the inventory format will change Needs re-review Needs testing
Projects
None yet
Milestone
No milestone
3 participants
@xprazak2 @ares @theforeman-bot