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.

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 #36982 - display short hostname on REX job pages #859

Merged
merged 1 commit into from
Feb 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/helpers/job_invocations_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def preview_hosts(template_invocation)
end

def collapsed_preview(target)
title = target.try(:name) || 'N/A'
title = target || 'N/A'
content_tag(:h5,
:class => "expander collapsed out",
:data => { :toggle => 'collapse',
Expand Down
4 changes: 2 additions & 2 deletions app/helpers/remote_execution_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def template_invocation_actions(task, host, job_invocation, template_invocation)
action: { href: current_host_details_path(host), 'data-method': 'get', id: "#{host.name}-actions-detail" } }
end
if authorized_for(controller: :job_invocations, action: :create) && (!host.infrastructure_host? || User.current.can?(:execute_jobs_on_infrastructure_hosts))
links << { title: (_('Rerun on %s') % host.name),
links << { title: (_('Rerun on %s') % host),
nofaralfasi marked this conversation as resolved.
Show resolved Hide resolved
action: { href: rerun_job_invocation_path(job_invocation, host_ids: [ host.id ]),
'data-method': 'get', id: "#{host.name}-actions-rerun" } }
end
Expand Down Expand Up @@ -274,7 +274,7 @@ def targeting_hosts(job_invocation, hosts)
task = template_invocation.try(:run_host_job_task)
link_authorized = !task.nil? && authorized_for(hash_for_template_invocation_path(:id => template_invocation).merge(:auth_object => host, :permission => :view_hosts, :authorizer => job_hosts_authorizer))

{ name: host.name,
{ name: host.to_label,
link: link_authorized ? template_invocation_path(:id => template_invocation) : '',
status: template_invocation_status(task, job_invocation.task),
actions: template_invocation_actions(task, host, job_invocation, template_invocation) }
Expand Down
9 changes: 7 additions & 2 deletions app/lib/actions/remote_execution/run_host_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ def inner_plan(job_invocation, host, template_invocation, proxy_selector, option
raise _('Could not use any template used in the job invocation') if template_invocation.blank?
features = template_invocation.template.remote_execution_features.pluck(:label).uniq
action_subject(host,
:host_display_name => host.to_label,
:job_category => job_invocation.job_category,
:description => job_invocation.description,
:job_invocation_id => job_invocation.id,
Expand Down Expand Up @@ -101,8 +102,12 @@ def live_output
end

def humanized_input
N_('%{description} on %{host}') % { :host => input[:host].try(:[], :name),
:description => input[:description].try(:capitalize) || input[:job_category] }
return unless input.present?

N_('%{description} on %{host}') % {
host: input[:host_display_name],
description: input[:description].try(:capitalize) || input[:job_category],
}
end

def humanized_name
Expand Down
2 changes: 1 addition & 1 deletion app/views/job_invocations/_preview_hosts_list.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
<% if @hosts.any? -%>
<ul>
<% @hosts.each do |host| -%>
<li><%= link_to h(host.name), current_host_details_path(host), :target => '_blank' %></li>
<li><%= link_to h(host), current_host_details_path(host), :target => '_blank' %></li>
<% end -%>

<% if @additional > 0 -%>
Expand Down
4 changes: 2 additions & 2 deletions app/views/template_invocations/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
:url => job_invocation_path(@template_invocation.job_invocation_id) }]

if @host
items << { :caption => _('Template Invocation for %s') % @host.name }
items << { :caption => _('Template Invocation for %s') % @host }
breadcrumbs(:resource_url => template_invocations_api_job_invocation_path(@template_invocation.job_invocation_id),
:name_field => 'host_name',
:switcher_item_url => template_invocation_path(':id'),
Expand All @@ -30,7 +30,7 @@ end
<% if @host %>
<% proxy_id = @template_invocation_task.input[:proxy_id] %>
<h3>
<%= _('Target: ') %><%= link_to(@host.name, current_host_details_path(@host)) %>
<%= _('Target: ') %><%= link_to(@host, current_host_details_path(@host)) %>
<% if proxy_id && proxy = SmartProxy.find_by(id: proxy_id) %>
<%= _('using Smart Proxy') %> <%= link_to(proxy.name, smart_proxy_path(proxy)) %>
<% end %>
Expand Down
9 changes: 7 additions & 2 deletions webpack/JobWizard/JobWizardSelectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,13 @@ export const selectHostsResponse = state => selectAPIResponse(state, HOSTS_API);
export const selectHostCount = state =>
selectHostsResponse(state).subtotal || 0;

export const selectHosts = state =>
(selectHostsResponse(state).results || []).map(host => host.name);
export const selectHosts = state => {
const hosts = selectHostsResponse(state).results || [];
return hosts.map(host => ({
name: host.name,
display_name: host.display_name,
}));
};

export const selectHostsMissingPermissions = state => {
const hostsResponse = selectHostsResponse(state);
Expand Down
6 changes: 3 additions & 3 deletions webpack/JobWizard/__tests__/fixtures.js
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,9 @@ export const gqlMock = [
hosts: {
totalCount: 3,
nodes: [
{ id: 'MDE6SG9zdC0x', name: 'host1' },
{ id: 'MDE6SG9zdC0y', name: 'host2' },
{ id: 'MDE6SG9zdC0z', name: 'host3' },
{ id: 'MDE6SG9zdC0x', name: 'host1', displayName: 'host1' },
{ id: 'MDE6SG9zdC0y', name: 'host2', displayName: 'host2' },
{ id: 'MDE6SG9zdC0z', name: 'host3', displayName: 'host3' },
],
},
},
Expand Down
12 changes: 8 additions & 4 deletions webpack/JobWizard/autofill.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,14 @@ export const useAutoFill = ({
handleSuccess: ({ data }) => {
setSelectedTargets(currentTargets => ({
...currentTargets,
hosts: (data.results || []).map(({ id, name }) => ({
id,
name,
})),
hosts: (data.results || []).map(
// eslint-disable-next-line camelcase
({ id, name, display_name }) => ({
id,
// eslint-disable-next-line camelcase
name: display_name || name,
})
),
nofaralfasi marked this conversation as resolved.
Show resolved Hide resolved
}));
},
})
Expand Down
6 changes: 3 additions & 3 deletions webpack/JobWizard/steps/HostsAndInputs/HostPreviewModal.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,16 @@ export const HostPreviewModal = ({ isOpen, setIsOpen, searchQuery }) => {
>
<List isPlain>
{hosts.map(host => (
<ListItem key={host}>
<ListItem key={host.name}>
<Button
component="a"
href={foremanUrl(`/hosts/${host}`)}
href={foremanUrl(`/hosts/${host.name}`)}
Copy link
Member

Choose a reason for hiding this comment

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

I'd be tempted to use host IDs in URLs, not names to avoid breaking when you rename hosts. I know Foreman is inconsistent about that. This is why I think we should avoid constructing URLs all over the place and instead rely on helpers. foremanUrl is really an anti-pattern.

If the UI relies on it so much, we should do what GH did and include html_url in the API response.

variant="link"
target="_blank"
rel="noreferrer"
isInline
>
{host}
{host.display_name}
</Button>
</ListItem>
))}
Expand Down
1 change: 1 addition & 0 deletions webpack/JobWizard/steps/HostsAndInputs/SelectGQL.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export const useNameSearchGQL = apiKey => {
data?.[dataName[apiKey]]?.nodes.map(node => ({
id: decodeId(node.id),
name: node.name,
displayName: node.displayName,
})) || [],
},
loading,
Expand Down
19 changes: 13 additions & 6 deletions webpack/JobWizard/steps/HostsAndInputs/SelectedChips.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { Chip, ChipGroup, Button } from '@patternfly/react-core';
import { sprintf, translate as __ } from 'foremanReact/common/I18n';
import { hostMethods } from '../../JobWizardConstants';

const SelectedChip = ({ selected, setSelected, categoryName }) => {
const SelectedChip = ({ selected, setSelected, categoryName, setLabel }) => {
const deleteItem = itemToRemove => {
setSelected(oldSelected =>
oldSelected.filter(({ id }) => id !== itemToRemove)
Expand All @@ -24,14 +24,14 @@ const SelectedChip = ({ selected, setSelected, categoryName }) => {
setSelected(() => []);
}}
>
{selected.map(({ name, id }, index) => (
{selected.map((result, index) => (
<Chip
key={index}
id={`${categoryName}-${id}`}
onClick={() => deleteItem(id)}
closeBtnAriaLabel={`Remove ${name}`}
id={`${categoryName}-${result.id}`}
onClick={() => deleteItem(result.id)}
closeBtnAriaLabel={`Remove ${result.name}`}
>
{name}
{setLabel(result)}
</Chip>
))}
</ChipGroup>
Expand All @@ -49,6 +49,7 @@ export const SelectedChips = ({
setSelectedHostGroups,
hostsSearchQuery,
clearSearch,
setLabel,
}) => {
const clearAll = () => {
setSelectedHosts(() => []);
Expand All @@ -67,16 +68,19 @@ export const SelectedChips = ({
selected={selectedHosts}
categoryName={hostMethods.hosts}
setSelected={setSelectedHosts}
setLabel={setLabel}
/>
<SelectedChip
selected={selectedHostCollections}
categoryName={hostMethods.hostCollections}
setSelected={setSelectedHostCollections}
setLabel={setLabel}
/>
<SelectedChip
selected={selectedHostGroups}
categoryName={hostMethods.hostGroups}
setSelected={setSelectedHostGroups}
setLabel={setLabel}
/>
<SelectedChip
selected={
Expand All @@ -86,6 +90,7 @@ export const SelectedChips = ({
}
categoryName={hostMethods.searchQuery}
setSelected={clearSearch}
setLabel={setLabel}
/>
{showClear && (
<Button variant="link" className="clear-chips" onClick={clearAll}>
Expand All @@ -105,10 +110,12 @@ SelectedChips.propTypes = {
setSelectedHostGroups: PropTypes.func.isRequired,
hostsSearchQuery: PropTypes.string.isRequired,
clearSearch: PropTypes.func.isRequired,
setLabel: PropTypes.func.isRequired,
};

SelectedChip.propTypes = {
categoryName: PropTypes.string.isRequired,
selected: PropTypes.array.isRequired,
setSelected: PropTypes.func.isRequired,
setLabel: PropTypes.func.isRequired,
};
1 change: 1 addition & 0 deletions webpack/JobWizard/steps/HostsAndInputs/hosts.gql
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ query($search: String!) {
nodes {
id
name
displayName
}
}
}
5 changes: 5 additions & 0 deletions webpack/JobWizard/steps/HostsAndInputs/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ const HostsAndInputs = ({
const dispatch = useDispatch();

const selectedHosts = selected.hosts;
const setLabel = result => result.displayName || result.name;
const setSelectedHosts = newSelected =>
setSelected(prevSelected => ({
...prevSelected,
Expand Down Expand Up @@ -191,6 +192,7 @@ const HostsAndInputs = ({
apiKey={HOSTS}
name="hosts"
placeholderText={__('Filter by hosts')}
setLabel={setLabel}
/>
)}
{hostMethod === hostMethods.hostCollections && (
Expand All @@ -201,6 +203,7 @@ const HostsAndInputs = ({
name="host collections"
url="/katello/api/host_collections?per_page=100"
placeholderText={__('Filter by host collections')}
setLabel={setLabel}
/>
)}
{hostMethod === hostMethods.hostGroups && (
Expand All @@ -210,6 +213,7 @@ const HostsAndInputs = ({
apiKey={HOST_GROUPS}
name="host groups"
placeholderText={__('Filter by host groups')}
setLabel={setLabel}
/>
)}
</InputGroup>
Expand All @@ -223,6 +227,7 @@ const HostsAndInputs = ({
setSelectedHostGroups={setSelectedHostGroups}
hostsSearchQuery={hostsSearchQuery}
clearSearch={clearSearch}
setLabel={setLabel}
/>
<Text>
{__('Apply to')}{' '}
Expand Down
5 changes: 3 additions & 2 deletions webpack/JobWizard/steps/ReviewDetails/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,13 @@ const ReviewDetails = ({

const hostsCount = useSelector(selectHostCount);
const [hostPreviewOpen, setHostPreviewOpen] = useState(false);
const NUM_CHIPS = 3;
const stringHosts = () => {
if (hosts.length === 0) {
return __('No Target Hosts');
}
if (hosts.length === 1 || hosts.length === 2) {
return hosts.join(', ');
if (hosts.length < NUM_CHIPS) {
return hosts.map(host => host.display_name).join(', ');
}
return (
<div>
Expand Down
4 changes: 3 additions & 1 deletion webpack/JobWizard/steps/form/SearchSelect.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export const SearchSelect = ({
apiKey,
url,
variant,
setLabel,
}) => {
const [onSearch, response, isLoading] = useNameSearch(apiKey, url);
const [isOpen, setIsOpen] = useState(false);
Expand Down Expand Up @@ -48,7 +49,7 @@ export const SearchSelect = ({
...selectOptions,
...Immutable.asMutable(response?.results || [])?.map((result, index) => (
<SelectOption key={index + 1} value={result.id}>
{result.name}
nofaralfasi marked this conversation as resolved.
Show resolved Hide resolved
{setLabel(result)}
</SelectOption>
)),
];
Expand Down Expand Up @@ -104,6 +105,7 @@ SearchSelect.propTypes = {
name: PropTypes.string,
selected: PropTypes.oneOfType([PropTypes.object, PropTypes.array]),
setSelected: PropTypes.func.isRequired,
setLabel: PropTypes.func.isRequired,
placeholderText: PropTypes.string,
apiKey: PropTypes.string.isRequired,
url: PropTypes.string,
Expand Down
2 changes: 2 additions & 0 deletions webpack/JobWizard/steps/form/__tests__/SelectSearch.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const apiKey = 'HOSTS_KEY';
describe('SearchSelect', () => {
it('too many', () => {
const onSearch = jest.fn();
const setLabel = jest.fn();
render(
<SearchSelect
selected={['hosts1,host2']}
Expand All @@ -19,6 +20,7 @@ describe('SearchSelect', () => {
{ results: ['host1', 'host2', 'host3'], subtotal: 101 },
false,
]}
setLabel={setLabel}
/>
);
const openSelectbutton = screen.getByRole('button', {
Expand Down
Loading