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

Resources health checks #3590

Merged
merged 10 commits into from
Apr 12, 2023
Merged

Resources health checks #3590

merged 10 commits into from
Apr 12, 2023

Conversation

luizbafilho
Copy link
Contributor

@luizbafilho luizbafilho commented Apr 3, 2023

Closes #2596

What changed?

  • Adds a health checker for some resources.
  • Initial support to:
    • Deployments
    • ReplicaSets
    • Pods

Why was this change made?

How was this change implemented?

How did you validate the change?

Release notes

Documentation Changes

@luizbafilho luizbafilho changed the title Working health checks Resources health checks Apr 4, 2023
@luizbafilho luizbafilho requested a review from joshri April 10, 2023 13:19
@joshri
Copy link
Contributor

joshri commented Apr 10, 2023

Front end looks okay but I can't test yet as OSS main appears to broken right now

@yiannistri
Copy link
Contributor

@joshri can you take another look now?

@joshri
Copy link
Contributor

joshri commented Apr 11, 2023

What’s the difference between health check and what we do for status? I’m mainly asking bc the front end code ends up looking very similar to what we use for the status indicator, and I’m wondering if it can be reused - at least the JSX should wind up the same to reuse styles.

Also - using the full message makes the column realllllly wide in the table. Is it correctly prioritized over status and message? Just expressing my overall desire to get rid of the horizontal scrolling

Are there plans to add this info to the detail pages? Or is this something that exists on non-primitives only?

@luizbafilho
Copy link
Contributor Author

luizbafilho commented Apr 11, 2023

What’s the difference between health check and what we do for status? I’m mainly asking bc the front end code ends up looking very similar to what we use for the status indicator, and I’m wondering if it can be reused - at least the JSX should wind up the same to reuse styles.

Also - using the full message makes the column realllllly wide in the table. Is it correctly prioritized over status and message? Just expressing my overall desire to get rid of the horizontal scrolling

Are there plans to add this info to the detail pages? Or is this something that exists on non-primitives only?

It is a replacement. Those are gonna be the source of truth for resource health.

Regarding the details page, I think the idea is to have an aggregator icon that would "consolidate" the whole app status, if all items in the inventory is green, there is a green icon on the top, and so on.

@TheGostKasper
Copy link
Contributor

[UI]
Both KubeStatusIndicator & HealthCheckStatusIndicator are pretty much the same in structure and naming Convension , different in dealing with props as the main diff is healthChecks doesn't deal with conditions to retrieve the text as it's in KubeStatusIndicator.
Why not just change KubeStatusIndicator to cover healthChecks ?

  • StatusIndicator is used everywhere in the system and doesn't worth the change .
  • Creating new component that is responsible for healthChecks is more reasonable to cover specific job as in the near future we will update to it and use it in other pages .

@joshri

@joshri
Copy link
Contributor

joshri commented Apr 11, 2023

Okay no worries - just trying to get some context as to what's going on. I was able to see the column and indicator for a Deployment in the UI, so consider the front end approved 👍

Copy link
Contributor

@yiannistri yiannistri left a comment

Choose a reason for hiding this comment

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

Looks good, I particularly liked how we go about testing this 👍

@luizbafilho luizbafilho enabled auto-merge (squash) April 12, 2023 15:28
@luizbafilho luizbafilho merged commit 2f6c246 into main Apr 12, 2023
@luizbafilho luizbafilho deleted the health-checks branch April 12, 2023 15:38
@joshri joshri mentioned this pull request Apr 12, 2023
@opudrovs opudrovs mentioned this pull request Apr 13, 2023
asaf400 pushed a commit to asaf400/weave-gitops that referenced this pull request Apr 16, 2023
---------

Co-authored-by: TheGostKasper <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate how to properly check the health status from k8s resources
5 participants