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

Refs #36517 - Correct host search for IPv6 on subnet list #9739

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Jun 19, 2023

The search linked to subnet.name= instead of subnet6.name, which meant there was nothing found.

This doesn't address the bug that it doesn't look at IPv6 subnets for the count.

@theforeman-bot
Copy link
Member

Issues: #36517

<td>
<%=
# TODO: this only works for IPv4
count = hosts_count[subnet]
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this comes from:

def hosts_count(resource_name = controller.resource_name)
@hosts_count ||= HostCounter.new(resource_name)
end

Which in turn comes from HostCounter.

The search linked to subnet.name= instead of subnet6.name, which meant
there was nothing found.

This doesn't address the bug that it doesn't look at IPv6 subnets for
the count.
@ekohl ekohl marked this pull request as ready for review June 19, 2023 10:43
@ekohl ekohl changed the title Fixes #36517 - Correct host count on IPv6 subnet list Refs #36517 - Correct host search for IPv6 on subnet list Jun 19, 2023
@stejskalleos stejskalleos self-assigned this Jun 23, 2023
Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

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

Works as expected, just one nit about the RM in the comment

<td><%= link_to_if_authorized(hosts_count[subnet], hash_for_hosts_path(:search => "subnet.name=\"#{subnet}\"")) %>
<td>
<%=
# TODO: https://projects.theforeman.org/issues/36517 this only works for IPv4
Copy link
Contributor

Choose a reason for hiding this comment

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

This TODO is linked to Redmine for this PR. Shouldn't there be a different Redmine issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this PR to be Refs, because I still think the real issue isn't solved. I could change this to be a unique RM issue just for the broken link, but if the count isn't correct I'm not sure anyone will click on it. The problem is that for every IPv6 subnet the count is 0. After this patch it will link to a listing that may be non-empty, but the label still says 0 so users may not click on it.

@stejskalleos stejskalleos merged commit 2eacedb into theforeman:develop Sep 19, 2023
7 checks passed
@stejskalleos
Copy link
Contributor

Thanks @ekohl

@ekohl ekohl deleted the fix-ipv6-subnet branch September 19, 2023 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants