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

Load selectable checks for a host #1600

Merged
merged 10 commits into from
Jul 11, 2023
Merged

Load selectable checks for a host #1600

merged 10 commits into from
Jul 11, 2023

Conversation

nelsonkopliku
Copy link
Member

@nelsonkopliku nelsonkopliku commented Jul 6, 2023

Description

First of a series of PRs that will bring checks to hosts.

The main thing that this change allows to do is to load the selectable checks for a specific host

image

You can browse to /hosts/:hostID/settings and see it loads the catalog for the host.

If running it locally make sure to have at least one check in wanda with env.target_type == "host" in the when condition, otherwise since there is no host checks currently available the page would be empty (story added)

image

We need to iterate multiple times to get to the final state of the design, so what we're focusing on now is making it work, then make it beautiful. Otherwise the PRs would be huge.

Next: add state/saga for the host check selection and trigger the API call when clicking on Save Check Selection.

How was this tested?

Automated tests.

@nelsonkopliku nelsonkopliku force-pushed the add-hosts-settings-page branch 4 times, most recently from 4ebcc91 to 217f7d9 Compare July 6, 2023 13:33
@nelsonkopliku nelsonkopliku changed the title wip Load selectable checks for a host Jul 6, 2023
@nelsonkopliku nelsonkopliku self-assigned this Jul 6, 2023
@nelsonkopliku nelsonkopliku added enhancement New feature or request user story javascript Pull requests that update Javascript code labels Jul 6, 2023
@nelsonkopliku nelsonkopliku marked this pull request as ready for review July 6, 2023 14:00
Copy link
Member

@EMaksy EMaksy left a comment

Choose a reason for hiding this comment

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

Hey man good work!

Maybe we can use One component for HostInfoBox and ClusterInfoBox in the future as they are similar and we could spare some css fights.

Besides this i left just some small comments but nothing strong minded --> LGTM

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

It looks good in general.
I would ask @jagabomb to visit the design of the check selection view.

Two side notes:

  • I would personally like to have a storybook story for the host settings, as it looks pretty easy to have
  • Having the HostSettings inside HostDetails would be more consistent, as we do this way in the clusters details view. Anyway, I don't know if some work is in progress for this, as there are many leftovers from previous CheckSelection refactor

assets/js/components/HostSettings/HostSettings.jsx Outdated Show resolved Hide resolved
@nelsonkopliku
Copy link
Member Author

Thanks @arbulu89 for the review.

Two side notes:

  • I would personally like to have a storybook story for the host settings, as it looks pretty easy to have

answered inline in the comment

  • Having the HostSettings inside HostDetails would be more consistent, as we do this way in the clusters details view.

I see where this comes from. I did that in light of our ADR about the frontend, however I reckon we're still not in the position to adhere that one. I can move back the HostSettings in HostDetails as we do for the cluster, however sooner or later that might be refactored.

Anyway, I don't know if some work is in progress for this, as there are many leftovers from previous CheckSelection refactor

So, well yes its a WIP and Jurgen would need to review more thoroughly as we get closer to his design, which we'd need to iterate over to get closer 😄

@nelsonkopliku
Copy link
Member Author

hey @arbulu89 I added an intermediate HostChecksSelection component and kept the new additions in HostDetails.
Test/stories added. For the UI thingy we'll iterate over with @jagabomb

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Thank you @nelsonkopliku ,
Some nitpicks. Feel free to ignore them XD
Ready to merge in any case

assets/js/trento.jsx Outdated Show resolved Hide resolved
assets/js/components/HostDetails/index.js Outdated Show resolved Hide resolved
assets/js/components/HostDetails/HostChecksSelection.jsx Outdated Show resolved Hide resolved
@nelsonkopliku nelsonkopliku merged commit 24dc985 into main Jul 11, 2023
16 checks passed
@nelsonkopliku nelsonkopliku deleted the add-hosts-settings-page branch July 11, 2023 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request javascript Pull requests that update Javascript code user story
Development

Successfully merging this pull request may close these issues.

3 participants