Feature #28836 - allow multiple disassociating of provisioning templates #502
Conversation
There was a problem hiding this comment.
Thanks, @yifatmakias! Works as expected (I was able to disassociate some templates) and tests pass, but I think it would be also nice to have this command similar to add-provisioning-templates with --provisioning-template-search. What do you think? Not blocker though.
|
|
||
| api_expects(:operatingsystems, :update) do |par| | ||
| par['id'] == '1' && | ||
| par['operatingsystem'] == { 'provisioning_template_ids' => [] } |
There was a problem hiding this comment.
Although the tests pass, this is weird, because in a real call we'd send an array with id ('provisioning_template_ids' => ['1'] in this case). Not sure why the test requires the array to be empty...
There was a problem hiding this comment.
You are right it should be 'provisioning_template_ids' => ['1'] in this case. I will fix it and also address the issue from the other PR you mentioned here.
|
I thought that when disassociating a template it is more likely to want to delete it according to the full name or specific id so there will be no mistake and that's why I thought that search option here is not necessary. But if you think it should be added I will add it. What do you say? @ofedoren |
|
@yifatmakias, I agree that search option has limited usage in this context, but if I wanted to disassociate all e.g. Kickstart templates, I'd rather use something like |
|
O.K, I will add the search option. |
There was a problem hiding this comment.
@yifatmakias, thanks! Now it's easier to disassociate multiple templates from os :)
Unfortunatelly, while testing --...-search option I've noticed that we have it quite broken and with associate command as well :(. It works, but the search result is limited to per_page setting in Foreman. Example: I've got an OS with 30 associated templates, 25 of which have prefix dis_. If I want to [dis]associate all the templates with this prefix, I will need to run a command with --...-search option twice in case I've got per_page set to 20 in Foreman. I've kinda resolved this issue as part of #417, but the solution works only on basic name resolving (uses --...-name[s] option).
Please let me know if you want to resolve this issue as part of this PR (which I think is out of scope) or we should create a separate issue in redmine. After we decide it I'll wait for new changes or merge this right away and we'll deal with the problem later :)
|
I agree that this problem is out of scope for this PR. I think we should create a new Redmine issue for this problem. |
|
@yifatmakias, okay then :) I've created the issue: https://projects.theforeman.org/issues/29231. So, merging this one, thanks! |
No description provided.