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 #31425 - Render statuses #8984

Conversation

kamils-iRonin
Copy link
Member

Reopened #8377

I have a feeling the previous PR was overcomplicated and I wanted to keep this one as simple as possible. It includes saving the status after each render attempt and displaying that on the template and host pages. I think this is more about the template than the host (we want to make sure that the templates are not broken), so I'm not really sure if host status is suitable here. Once this is merged, we can think about some kind of monitoring/notifications.

Here are some screenshots:

image
image
image

@theforeman-bot
Copy link
Member

Issues: #31425

@kamils-iRonin kamils-iRonin force-pushed the 31425-templates-rendering-host-status-3 branch 4 times, most recently from f3fb9d2 to d8f24e5 Compare December 10, 2021 14:50
@amirfefer
Copy link
Member

amirfefer commented Dec 13, 2021

Thanks @kamils-iRonin!

I'm not sure if the card "Render Statuses" fits the overview tab, it has too much data IMHO which takes much of the screen's real estate, the overview tab mostly shows recent / aggregated information regarding the host.

@MariSvirik
Copy link

Hey, @kamils-iRonin it's a great effort.
I agree with what @amirfefer said.
Still, I have a couple of questions about render statuses as I'm hearing about it for the first time. (Apologies for a lack of my technical knowledge)
What are render statuses? Do they tell a user if a template rendered correctly?
How is it important for a user? And is it important in the context of a specific host?

@kamils-iRonin
Copy link
Member Author

@amirfefer @MariSvirik thanks for the feedback! I'll try to address the questions below, do not hesitate to ask if something is still unclear 😄

What are render statuses?

Basically, the render statuses are designed to provide information about possible rendering problems. We would like to know which templates are used with which hosts and if any errors occurred during the rendering process.

Do they tell a user if a template rendered correctly? How is it important for a user?

Exactly, with this we can prevent further problems, if we detect that there is a problem with rendering some template, we can check which hosts may be affected. Also, if you use the renderer in unsafe mode, it can be helpful to migrate to safe mode.

And is it important in the context of a specific host?

The first idea was to introduce a new host status, but now I think it's more about the template. I don't have a strong opinion on whether the render statuses should be displayed on the host page or not.

I'm not sure if the card "Render Statuses" fits the overview tab, it has too much data IMHO which takes much of the screen's real estate.

Maybe it would be enough to show how many templates were rendered for this particular host and if all renderings were successful? (we can also add a "Details" button)

@ares
Copy link
Member

ares commented Dec 15, 2021

Great work so far! I'd say the templates would deserve it's own tab. Either for this table with perhaps more info (link to audits of this template, info about it's type etc) or it should be a part of a new Provisioning tab. We can start with Templates tab and then perhaps move it if it would be better. Just my 2 cents

@kamils-iRonin kamils-iRonin force-pushed the 31425-templates-rendering-host-status-3 branch from 5aeebc8 to 0100b65 Compare February 3, 2022 17:47
@Ron-Lavi
Copy link
Member

Ron-Lavi commented Apr 3, 2022

Hey @kamils-iRonin what is the status here?
is it just waiting for another review?

@kamils-iRonin
Copy link
Member Author

@Ron-Lavi If I remember correctly, there are minor things to improve and tests to fix. I'm quite busy with another project at the moment, but I'll try to finish it in the next few days.

@Ron-Lavi Ron-Lavi marked this pull request as draft April 12, 2022 16:33
@kamils-iRonin kamils-iRonin force-pushed the 31425-templates-rendering-host-status-3 branch 3 times, most recently from 24a8c50 to 3d9e63a Compare May 17, 2022 19:54
@kamils-iRonin kamils-iRonin force-pushed the 31425-templates-rendering-host-status-3 branch from 3d9e63a to cc5bb6a Compare May 19, 2022 07:55
@kamils-iRonin
Copy link
Member Author

I believe the failed JavaScript tests are not related, I just ran the tests on develop and I see the same errors

@kamils-iRonin
Copy link
Member Author

[test foreman]

@kamils-iRonin kamils-iRonin force-pushed the 31425-templates-rendering-host-status-3 branch from cc5bb6a to c484d4c Compare May 23, 2022 20:59
@kamils-iRonin kamils-iRonin marked this pull request as ready for review May 25, 2022 14:47
@kamils-iRonin
Copy link
Member Author

@Ron-Lavi I think it's ready for another review

@kamils-iRonin kamils-iRonin force-pushed the 31425-templates-rendering-host-status-3 branch from c484d4c to a1f2f71 Compare May 30, 2022 06:58
@ezr-ondrej ezr-ondrej removed their assignment Jun 2, 2022
@kamils-iRonin kamils-iRonin force-pushed the 31425-templates-rendering-host-status-3 branch from a1f2f71 to dc5f00e Compare June 14, 2022 12:03
@Ron-Lavi Ron-Lavi force-pushed the 31425-templates-rendering-host-status-3 branch from dc5f00e to f92a74b Compare October 2, 2022 06:19
@Ron-Lavi
Copy link
Member

Ron-Lavi commented Oct 2, 2022

Hey @kamils-iRonin, I guess this PR fell through the cracks,
I rebased it for you and fixed the conflict with the safemode UI setting change

@kamils-iRonin kamils-iRonin force-pushed the 31425-templates-rendering-host-status-3 branch from f92a74b to 409c027 Compare October 13, 2022 08:20
@kamils-iRonin kamils-iRonin force-pushed the 31425-templates-rendering-host-status-3 branch from 409c027 to 1e9ba24 Compare November 10, 2022 08:28
@kamils-iRonin
Copy link
Member Author

Hi @Ron-Lavi, thanks and I think it is ready now

@sbernhard
Copy link
Contributor

What shall we do with this PR? Rebase and finish or close it? @MariaAga ?

@kamils-iRonin kamils-iRonin force-pushed the 31425-templates-rendering-host-status-3 branch from 1e9ba24 to 1bed7b1 Compare December 12, 2023 13:25
@kamils-iRonin
Copy link
Member Author

It is quite old PR and the details are not entirely fresh in my memory. Just in case I have rebased and tested. It seems to work fine.

@kamils-iRonin kamils-iRonin force-pushed the 31425-templates-rendering-host-status-3 branch from 1bed7b1 to 7ccdc13 Compare December 12, 2023 15:19
@kmalyjur
Copy link
Contributor

Since this PR is quite old, I just want to let you know that I'm going to take a look at it within the next few days.

@kamils-iRonin kamils-iRonin force-pushed the 31425-templates-rendering-host-status-3 branch from 7ccdc13 to b1f012b Compare February 15, 2024 19:14
Copy link
Contributor

@kmalyjur kmalyjur left a comment

Choose a reason for hiding this comment

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

@adamruzicka helped me with the review (mainly with the backend) and we came to the conclusion that it might be worth adding the same "Render Statuses" tab to the Host Groups and also to the Provisioning templates card in the new host detail page -> Details (sort of like the Overview -> Recent jobs card), however, I know that this has already been discussed and it's possible it was decided not to include it there.
Idk about the tests, hopefully rebasing would help.

Comment on lines +1 to +3
object @render_status

attributes :id
Copy link
Contributor

Choose a reason for hiding this comment

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

This only seems to be used to extend the main.json.rabl. Does it need to be a separate file?

import { translate as __ } from '../../common/I18n';
import { STATUS } from '../../constants';

const Empty = ({ responseStatus }) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have /common/EmptyState in Foreman, couldn't you please use that instead of creating a new component?

page={page}
onSetPage={(_event, newPage) => setPage(newPage)}
onPerPageSelect={(_event, newPerPage) => updatePerPage(newPerPage)}
widgetId="pagination-options-menu-top"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we use widgetId

Comment on lines +81 to +94
style={{ fill: 'var(--pf-global--success-color--100)' }}
/>
) : (
<ExclamationCircleIcon
style={{ fill: 'var(--pf-global--danger-color--100)' }}
/>
),
safemode: () =>
safemode ? (
<Fragment />
) : (
<Tooltip position="right" content={__('Unsafe Mode')}>
<ExclamationTriangleIcon
style={{ fill: 'var(--pf-global--warning-color--100)' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please import colors like this:
import { global_success_color_100 as successedColor } from '@patternfly/react-tokens';
And then you can use them this way:
<CheckCircleIcon color={successedColor.value} />

setPage(1);
};

const columnLabels = {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is some issue with the labels. It looks like this when the screen is too small:
Screenshot from 2024-03-14 14-31-38

Copy link

Thank you for your contribution! This PR has been inactive for 3 months, closing for now. Feel free to reopen when you return to it. This is an automated process.

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

Successfully merging this pull request may close these issues.

9 participants