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 #31792 - make the configuration status clickable #8305

Merged
merged 1 commit into from Feb 8, 2021

Conversation

ares
Copy link
Member

@ares ares commented Feb 2, 2021

The host substatuses can optionally define a link. In case of
configuration status, it's handy to link the last configuration report
for the host. This PR adds exactly that. Since reports can be deleted
while the status may still exists, we don't link anything in case there
no report. Also we don't set the link in case the user does not have
permissions to see the config report.

The host substatuses can optionally define a link. In case of
configuration status, it's handy to link the last configuration report
for the host. This PR adds exactly that. Since reports can be deleted
while the status may still exists, we don't link anything in case there
no report. Also we don't set the link in case the user does not have
permissions to see the config report.
@theforeman-bot
Copy link
Member

Issues: #31792

@ares
Copy link
Member Author

ares commented Feb 2, 2021

config_status

@ezr-ondrej ezr-ondrej self-assigned this Feb 3, 2021
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

As a user I first saw the screenshot and thought about where it would link. I would have expected it to link to all reports for that host, not the last one. It has the added benefit that it's a static link without the need to do a database query.

As a user it may also be confusing that sometimes I can see a report link, but sometimes I don't. Then I wouldn't know if I didn't have reports or just no permission.

Do you agree?

@ares
Copy link
Member Author

ares commented Feb 3, 2021

The logic should be imho consitent with elsehere, rex status takes you to the last job, compliance status takes you to the last arf report. The reason behind it is, only the last one determines the status value.

Also (less relevant but still). There's a reports button already on the hosts detail page and there's a button to "show other reports for this host" in the report detail, so it's easy to navigate there. There's no way to navigate directly to the report that caused this change now though.

I think the story/workflow really is - As a user I want to see why the host configuration failed. That happened to me yesterday :-)

@ekohl
Copy link
Member

ekohl commented Feb 3, 2021

So for failed I think your workflow makes sense. On the screenshot I saw out of sync and in that case the last report is less useful. This stuff is hard.

@ares
Copy link
Member Author

ares commented Feb 4, 2021

Even for out of sync, I probably want to see when the last one came in so I know for when to search in logs. Also I want to know what has changed in the last report that killed the agent/host.

@ezr-ondrej
Copy link
Member

So for failed I think your workflow makes sense. On the screenshot I saw out of sync and in that case the last report is less useful. This stuff is hard.

I'd agree here, but IMHO it could be confusing if we go to list of reports on Out of sync and to the last if last is failed, I prefer this.

Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

Thanks @ares, works for me 👍

@ekohl
Copy link
Member

ekohl commented Feb 4, 2021

Just mentally going through different situations.

With out of sync it's also possible that the last report was already deleted. Since it checks whether there's a last report, it won't show a link.

With different report sources, is it possible the host is considered out of sync due to Puppet but the last report comes from Ansible and is ok? Similar, a failed Puppet report but the last report is from Ansible and is ok.

To be clear, I know the frustration of trying to get to the right report. I always get annoyed that the last failed reports on the dashboard widget doesn't link directly to that report but rather a list of reports for that host.

This should all be considered in the new host detail page. IMHO that should show the x reports and also allow filtering on the last x eventful reports.

As for this PR: if you can answer the different report sources question I'm fine with merging this. While not perfect, it can help users until we have a new host detail page.

@ezr-ondrej
Copy link
Member

ezr-ondrej commented Feb 8, 2021

With out of sync it's also possible that the last report was already deleted. Since it checks whether there's a last report, it won't show a link.

Probably ok, isn't it?

With different report sources, is it possible the host is considered out of sync due to Puppet but the last report comes from Ansible and is ok? Similar, a failed Puppet report but the last report is from Ansible and is ok.

I don't think it's possible. after a ReportImporter#import, host_statuses gets refreshed and the ConfigurationStatus is using the last_report's origin to determine whether it's out_of_sync? So if there is later puppet report, and newer Ansible report, the Puppet one gets ignored and the other way around.

I get it's not perfect solution and we may do better, but I guess that should be considered in new host detail, here I think we are good to go as is. I don't think there are major downsides of this and IMHO only real corner cases may cause confusion (and only valid show stopper for this would be if we would introduce some confusion. I don't consider "we might be more helpful" a show stopper.)

@ekohl
Copy link
Member

ekohl commented Feb 8, 2021

With out of sync it's also possible that the last report was already deleted. Since it checks whether there's a last report, it won't show a link.

Probably ok, isn't it?

Yes, though I can imagine some user frustration why it sometimes is and sometimes isn't a link. That should be input to think about in the new host detail page.

I don't think it's possible. after a ReportImporter#import, host_statuses gets refreshed and the ConfigurationStatus is using the last_report's origin to determine whether it's out_of_sync? So if there is later puppet report, and newer Ansible report, the Puppet one gets ignored and the other way around.

As a Puppet user, that sounds like a bug to me. If I'm a Puppet user, I probably wouldn't use Ansible in a sync fashion like Puppet. If the Ansible report comes in but Puppet is still out of sync, that would be something that needs fixing. However, I should note that I think most users don't think about Ansible as state management that gets in sync. So IMHO by default it should not factor in to the sync calculation.

@ezr-ondrej
Copy link
Member

ezr-ondrej commented Feb 8, 2021

So IMHO by default it should not factor in to the sync calculation.

It is one of the improvements to Ansible that should happen in 2.5/2.6 timeline.

Though it is already happening today, so there is nothing to do about it. And it has easy workaround (if you don't need ansible facts - what's probably the case if you use puppet, you just do not setup the ansible callback)

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

With those things noted, I think this can be added. Feels like a nice thing to demo but I doubt @ares will have time. @ezr-ondrej do you think you can demo it?

@ezr-ondrej
Copy link
Member

Sure, I can demo it ;)

@ezr-ondrej ezr-ondrej merged commit ccbc96d into theforeman:develop Feb 8, 2021
@ares
Copy link
Member Author

ares commented Mar 23, 2021

Sorry for the late answer. There is no issue if people use both Ansible and Puppet. Ansible does not create a config report on any other run than when applying assign roles. Only such ansible run is considered a cfgmgmt run. And in that case, it creates the report and is the report to show. The status is always determined by the last report. So if someone combines two cfgmgmt agents, they are still interested in the last report that caused the last status change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants