Fixes #28287 - Import Ansible roles and variables in one step #379
Conversation
webpack/components/AnsibleRolesAndVariables/AnsibleRolesAndVariables.js
Outdated
Show resolved
Hide resolved
webpack/components/AnsibleRolesAndVariables/AnsibleRolesAndVariables.js
Outdated
Show resolved
Hide resolved
| </Table> | ||
| {renderNotFound()} | ||
| {renderPagination()} | ||
| {renderSubmitAndCancel()} |
There was a problem hiding this comment.
You can turn these into separate components to make this component a bit more slim.
|
Nice work, I added a couple of comments. |
shiramax
force-pushed
the
import
branch
2 times, most recently
from
083d46d
to
b86bf4f
Compare
|
Nice progress, the pagination does not quite work for me:
nit: it could be nice to add a bit of space between the buttons |
webpack/components/AnsibleRolesAndVariables/AnsibleRolesAndVariables.js
Outdated
Show resolved
Hide resolved
shiramax
force-pushed
the
import
branch
2 times, most recently
from
d500a7b
to
ef4c3e4
Compare
|
@xprazak2 I've addressed all your comments, let me know if you have more comments. thanks! |
|
I still see roles/variables repeatedly offered for update. Reproducer:
role = AnsibleRole.create(:name => "another.role")
AnsibleVariable.create(
:key => "another_role_variable",
:value => "foo",
:hidden_value => true,
:imported => true,
:ansilbe_role_id => role.id
)
|
| import_variable_names([]).each do |kind, variables| | ||
| variables.each do |variable| | ||
| next unless roles['old'].values.map { |role| role['id'] }.include?(variable.ansible_role_id) | ||
| role = AnsibleRole.where(:id => variable.ansible_role_id)[0] |
There was a problem hiding this comment.
nit: role = variable.ansible_role ?
|
@xprazak2 thanks now I'm able to reproduce it, it was fixed. |
|
[test foreman_ansible] |
|
Works as expected, patternfly packages should not be in devDependencies, but I have no additional comments other than that. |
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
I didn't tested it, but code looks reasonable, if you want mi to take it for a spin, give me a shout |
|
Maybe it would deserve some commit description? 🤷 |
| @@ -22,18 +22,19 @@ def destroy | |||
|
|
|||
| def import | |||
| changed = @importer.import! | |||
| if changed.values.all?(&:empty?) | |||
| @rows = prepare_ansible_import_rows(changed, @variables_importer) | |||
| if @rows.empty? | |||
| success no_changed_roles_message | |||
There was a problem hiding this comment.
this message could be tailored into "No added or removed roles nor variables" I'm ok to do so in another PR though :)
This PR Includes: - The ability to sync roles and their variables from Capsule in one step - React table to present all the roles and variables that need to be synced. - Removing the 'import variables' view from the index variables page.
There was a problem hiding this comment.
All of my comments are addressed and all works as expected. I'm not merging since I see red tests, but if that's unrelated, anyone please go ahead and click the green button. Thanks @shiramax this is really a nice improvement! Looking forward to see it on the next community demo :-)
|
Thanks everyone! |
This PR Includes: