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

Add a filter on recommendation #366

Closed

Conversation

benjlevesque
Copy link

As suggested in #321, this PR adds a filter to hide containers that matches the VPA recommendation.

The proposed filters are:

  • all: current behaviour
  • any: show containers that don't meet recommendations
  • guaranteed: show containers that don't meet guaranteed recommendations
  • burstable: show containers that don't meet burstable recommendations

UI:

image

As I am a new user of goldilocks, let me know if those filters are not the correct ones.

@sudermanjr
Copy link
Member

First glance this looks really great. Thanks for the contribution!

I'll try to give it a more thorough review this week.

Copy link
Member

@sudermanjr sudermanjr left a comment

Choose a reason for hiding this comment

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

I think there might be an issue with the filter logic. Just testing functionally, I get this

▶ ./goldilocks summary --filter any | jq .
{
  "Namespaces": {
    "default": {
      "namespace": "default",
      "deployments": {}
    }
  },
  "Filter": "any"
}

▶ ./goldilocks summary --filter burstable | jq .
{
  "Namespaces": {
    "default": {
      "namespace": "default",
      "deployments": {}
    }
  },
  "Filter": "burstable"
}

▶ ./goldilocks summary --filter guaranteed | jq .
{
  "Namespaces": {
    "default": {
      "namespace": "default",
      "deployments": {
        "demo-basic-demo": {
          "deploymentName": "demo-basic-demo",
          "containers": {
            "basic-demo": {
              "containerName": "basic-demo",
              "lowerBound": {
                "cpu": "15m",

(output was shortened)

TL;DR - I see recommendations for guaranteed, but not any. I had thought that "any" should include the "guaranteed", but maybe I'm off here?

Copy link
Member

@sudermanjr sudermanjr left a comment

Choose a reason for hiding this comment

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

Also, please see my previous comment regarding the filter logic.

I have additional concerns about the implementation of the UI, but it's going to take me a bit to compile them

@@ -0,0 +1,38 @@
{{ define "selected-filter" }}
{{ if . }}
<i aria-hidden="true" class="fas fa-fw fa-check"></i>
Copy link
Member

Choose a reason for hiding this comment

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

The aria-hidden will make this check invisible to screen readers.

Suggested change
<i aria-hidden="true" class="fas fa-fw fa-check"></i>
<i class="fas fa-fw fa-check"></i>

@github-actions github-actions bot added the stale Marked as stale by stalebot label Jan 13, 2022
@github-actions github-actions bot closed this Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Marked as stale by stalebot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants