-
Notifications
You must be signed in to change notification settings - Fork 112
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
Fixes #37228 - Replace Ansible/Roles page with React component #696
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,15 @@ | ||
object @ansible_role | ||
|
||
extends "api/v2/ansible_roles/show" | ||
attributes :id, :name, :updated_at | ||
code :hostgroups_count do |role| | ||
role.hostgroups.count | ||
end | ||
code :hosts_count do |role| | ||
role.hosts.count | ||
end | ||
code :variables_count do |role| | ||
role.ansible_variables.count | ||
end | ||
code :can_delete do | ||
User.current.can?(:destroy_ansible_roles) | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,19 +2,4 @@ | |
|
||
require 'test_plugin_helper' | ||
# functional tests for AnsibleRolesController | ||
class AnsibleRolesControllerTest < ActionController::TestCase | ||
setup do | ||
@role = FactoryBot.create(:ansible_role) | ||
end | ||
|
||
basic_index_test | ||
|
||
test 'should destroy role' do | ||
assert_difference('AnsibleRole.count', -1) do | ||
delete :destroy, | ||
:params => { :id => @role.id }, | ||
:session => set_session_user | ||
end | ||
assert_redirected_to ansible_roles_url | ||
end | ||
end | ||
class AnsibleRolesControllerTest < ActionController::TestCase; end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's an empty class, is there any point in keeping it at all? I'd also note that there are system tests (https://guides.rubyonrails.org/testing.html#system-testing) that can use a real browser. I'd love to still have some tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose I could remove it, but I would like to have a test for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fair. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,4 +11,11 @@ class UiAnsibleRolesControllerTest < ActionController::TestCase | |
res = JSON.parse @response.body | ||
assert_equal res['total'], res['results'].size | ||
end | ||
|
||
test 'should destroy role' do | ||
delete :destroy, | ||
:params => { :id => @role.id }, | ||
:session => set_session_user | ||
assert_response :success | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a test to verify that a user without the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure if this is necessary. The permissions should already be tested by a test in core and I didn't really find an existing test that tested this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MariaAga what are your thoughts about this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not my area, but I couldnt find a test in core for it in core |
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
import React from 'react'; | ||
import { useAPI } from 'foremanReact/common/hooks/API/APIHooks'; | ||
import TableIndexPage from 'foremanReact/components/PF4/TableIndexPage/TableIndexPage'; | ||
import RelativeDateTime from 'foremanReact/components/common/dates/RelativeDateTime'; | ||
import { translate as __ } from 'foremanReact/common/I18n'; | ||
import { getDocsURL } from 'foremanReact/common/helpers'; | ||
import { | ||
hostgroupsLink, | ||
hostsLink, | ||
variablesLink, | ||
} from './AnsibleRolesPageHelpers'; | ||
import { AnsibleRolesImportButtonWrapper } from './components/AnsibleRolesImportButtonWrapper'; | ||
import { showToast } from '../../toastHelper'; | ||
|
||
export const AnsibleRolesPage = () => { | ||
const { | ||
response: { results }, | ||
status, | ||
} = useAPI('get', '/api/v2/permissions/current_permissions'); | ||
|
||
if (status === 'ERROR') { | ||
showToast({ | ||
type: 'error', | ||
message: __('There was an error requesting user permissions'), | ||
}); | ||
} | ||
|
||
return ( | ||
<TableIndexPage | ||
header={__('Ansible Roles')} | ||
controller="ansible_roles" | ||
apiUrl="/ansible/ui_ansible_roles" | ||
apiOptions={{ key: 'ANSIBLE_ROLES_API_REQUEST_KEY' }} | ||
hasHelpPage | ||
creatable={false} | ||
isDeleteable | ||
customToolbarItems={ | ||
<AnsibleRolesImportButtonWrapper | ||
apiStatus={status} | ||
allPermissions={results} | ||
/> | ||
} | ||
customHelpURL={getDocsURL( | ||
'Managing_Configurations_Ansible', | ||
'Importing_Ansible_Roles_and_Variables_ansible' | ||
)} | ||
columns={{ | ||
name: { title: __('Name'), isSorted: true, weight: 0 }, | ||
hostgroups_count: { | ||
title: __('Hostgroups'), | ||
weight: 1, | ||
wrapper: ({ name, hostgroups_count: hostgroupsCount }) => | ||
hostgroupsLink(name, hostgroupsCount, status, results), | ||
}, | ||
hosts_count: { | ||
title: __('Hosts'), | ||
weight: 2, | ||
wrapper: ({ name, hosts_count: hostsCount }) => | ||
hostsLink(name, hostsCount, status, results), | ||
}, | ||
variables_count: { | ||
title: __('Variables'), | ||
weight: 3, | ||
wrapper: ({ name, variables_count: variablesCount }) => | ||
variablesLink(name, variablesCount, status, results), | ||
}, | ||
updated_at: { | ||
title: __('Imported at'), | ||
isSorted: true, | ||
weight: 4, | ||
wrapper: ({ updated_at: updatedAt }) => ( | ||
<RelativeDateTime date={new Date(updatedAt)} /> | ||
), | ||
}, | ||
}} | ||
/> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
import React from 'react'; | ||
|
||
export const hostgroupsLink = ( | ||
roleName, | ||
hostgroupsCount, | ||
apiStatus, | ||
permissions | ||
) => { | ||
if ( | ||
apiStatus === 'RESOLVED' && | ||
permissions.some(perm => perm.name === 'view_hostgroups') | ||
) { | ||
return link(hostgroupsUrl(roleName), hostgroupsCount); | ||
} | ||
return hostgroupsCount; | ||
}; | ||
|
||
export const hostsLink = ( | ||
roleName, | ||
hostgroupsCount, | ||
apiStatus, | ||
permissions | ||
) => { | ||
if ( | ||
apiStatus === 'RESOLVED' && | ||
permissions.some(perm => perm.name === 'view_hosts') | ||
) { | ||
return link(hostsUrl(roleName), hostgroupsCount); | ||
} | ||
return hostgroupsCount; | ||
}; | ||
|
||
export const variablesLink = ( | ||
roleName, | ||
hostgroupsCount, | ||
apiStatus, | ||
permissions | ||
) => { | ||
if ( | ||
apiStatus === 'RESOLVED' && | ||
permissions.some(perm => perm.name === 'view_ansible_variables') | ||
) { | ||
return link(variablesUrl(roleName), hostgroupsCount); | ||
} | ||
return hostgroupsCount; | ||
}; | ||
|
||
const link = (url, displayText) => ( | ||
<a href={url} target="_blank" rel="noreferrer"> | ||
{displayText} | ||
</a> | ||
); | ||
|
||
const hostgroupsUrl = roleName => `/hostgroups${searchString(roleName)}`; | ||
const hostsUrl = roleName => `/new/hosts${searchString(roleName)}`; | ||
const variablesUrl = roleName => `ansible_variables${searchString(roleName)}`; | ||
const searchString = roleName => | ||
`?search=${encodeURIComponent(`ansible_role = ${roleName}`)}`; | ||
|
||
export const importUrl = (proxyName, proxyId) => | ||
`ansible_roles/import?proxy=${encodeURIComponent(`${proxyId}-${proxyName}`)}`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
export const testSmartProxies = [ | ||
{ | ||
created_at: '2024-06-21 14:49:35 +0200', | ||
updated_at: '2024-06-21 14:49:35 +0200', | ||
hosts_count: 0, | ||
name: 'debuggable', | ||
id: 2, | ||
url: 'http://smart-proxy-ansible-capable.example.com:8080', | ||
remote_execution_pubkey: null, | ||
download_policy: 'on_demand', | ||
supported_pulp_types: [], | ||
lifecycle_environments: [], | ||
features: [ | ||
{ | ||
capabilities: ['ansible-runner', 'single'], | ||
name: 'Dynflow', | ||
id: 17, | ||
}, | ||
{ | ||
capabilities: ['vcs_clone'], | ||
name: 'Ansible', | ||
id: 20, | ||
}, | ||
{ | ||
capabilities: [], | ||
name: 'Logs', | ||
id: 13, | ||
}, | ||
], | ||
}, | ||
]; |
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 don't see any permission filtering here. Am I missing something?
Also, couldn't this just use the regular REST API to delete roles?
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.
Indeed, I forgot to add the permission.
The table component uses the same endpoint for querying and deleting items, so it's unfortunately not possible to use the API just for deletion.
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.
Are we sure this is the right approach? Adding an API call here doesn't seem right to me. I know it might be simpler, but we can still use the existing API call to delete a role. It might require some frontend adjustments, but it will align with the rest of our code.
For reference, this is how we do it on the new hosts page: https://github.com/theforeman/foreman/blame/6aac916fb183f47596e8d543756faac27750a21e/webpack/assets/javascripts/react_app/components/HostsIndex/index.js#L283
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.
Unfortunately, this would require significant changes to TableIndexPage.js and Table.js in core, as the idea is that TableIndexPage represents an interface to a single controller that has the necessary CRUD methods.
One could add another prop,
deletionURL
to TableIndexPage and Table that would take precedence over the original URL for deletion but I am not sure that is a cleaner solution.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't we add the
Delete
option to therowKebabItems
action items instead of using theisDeleteable
property? From there, we can use the URL of the existing API for the delete action.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.
Unfortunately, we would lose the delete modal that way.