Skip to content

Conversation

@debsmita1
Copy link
Contributor

@debsmita1 debsmita1 commented Jan 20, 2020

This PR includes:

  • the resources tab, which lists all the resources that gets created when a helm chart is installed
  • unit tests

Refer Jira story: https://issues.redhat.com/browse/ODC-2701
Screenshot from 2020-01-20 22-50-12

GIF:
final_5e2a6f1f05e9760016f86ab9_610183

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 20, 2020
@debsmita1
Copy link
Contributor Author

/cc @rohitkrai03 @christianvogt

@andrewballantyne
Copy link
Contributor

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 21, 2020
@debsmita1
Copy link
Contributor Author

/retest

@debsmita1 debsmita1 force-pushed the helm-release-resources branch from 1893f21 to df16e43 Compare January 21, 2020 08:17
@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Jan 21, 2020
@debsmita1
Copy link
Contributor Author

^ @openshift/team-devconsole-ux

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 21, 2020
@debsmita1 debsmita1 force-pushed the helm-release-resources branch from df16e43 to b99efcf Compare January 21, 2020 16:17
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 21, 2020
Copy link
Contributor

Choose a reason for hiding this comment

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

@debsmita1 can't we use className here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

style is a required prop of the TableRow component

Copy link
Contributor

Choose a reason for hiding this comment

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

types or filters and resources? you can specify the return type as well

Copy link
Contributor

Choose a reason for hiding this comment

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

type for resource and function return type as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this customKind needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am reading from a secret resource to display the details of a Helm Release. If I don't send this customKind prop, then it will display the Secret kind in the Resource Icon instead of Helm Release

Comment on lines 11 to 15
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem right here. You can probably use Object.keys(resources).reduce here. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

use optional chaining here resourceDetails?.metadata?.labels?.['name']

Copy link
Contributor

Choose a reason for hiding this comment

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

@debsmita1 I'm guessing metadata.labels.name is same as the metadata.name? or is it different?

Copy link
Contributor Author

@debsmita1 debsmita1 Jan 23, 2020

Choose a reason for hiding this comment

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

@sahil143 it's different. metadata.name is the name of the secret resource & metadata.labels.name is the helm release name

Comment on lines +17 to +47
Copy link
Contributor

Choose a reason for hiding this comment

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

@debsmita1 @serenamarie125 how did we come up with this subset of resources? We cannot find EVERY resource that is in the helm release by using our standard client APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @siamaksade
I'm assuming we want to see all resources in a helm release?

Choose a reason for hiding this comment

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

@christianvogt yes, all resources in a helm release

@christianvogt
Copy link
Contributor

@debsmita1 you are using the label selector release=<release name> to identify which resources are in a helm release. This label is only added by helm classic according to the docs: https://github.com/helm/helm-classic/blob/master/docs/using_labels.md

Meanwhile the current helm docs state that no such labels are present: https://helm.sh/docs/topics/chart_best_practices/labels/

We need to investigate further another way to identify resources in helm release.

@sahil143
Copy link
Contributor

@christianvogt We should be checking a number of labels. There should be utilitiy isHelmReleaseNode It checks for Heritage=Helm or chart=<chart-name> label to determine if a resource is helm or not. But if release=<release-name> is not present then we might have a problem with Helm Grouping also because we use the release label to group the Helm nodes in a group.

@debsmita1 debsmita1 force-pushed the helm-release-resources branch from b99efcf to 837d686 Compare January 23, 2020 12:10
@debsmita1 debsmita1 requested a review from sahil143 January 23, 2020 12:11
@siamaksade
Copy link

@christianvogt @sahil143 the labels are not set automatically in Helm 3 AFAIK. The combination of chart and release labels is not a fail-safe mechanism for finding the resources given that it's up to the chart author to set them in all the resource templates, even though that's the recommendation.

The Helm Release secret has a manifest attribute that contains all the resources that are created by that particular release. For example this is the content of the manifest attribute for a wordpress release: https://pastebin.com/raw/DZDvdCpr

@debsmita1 debsmita1 force-pushed the helm-release-resources branch from 837d686 to 60d2b45 Compare January 23, 2020 17:52
@christianvogt
Copy link
Contributor

We cannot use the manifest right now because we'll need backend api for it.
We have no good way to list the resources other than choosing a finite set and also only showing v2 charts that added the labels.

It won't work for v3 helm and will also mislead the user on which resources were created.

@sbose78
Copy link

sbose78 commented Jan 24, 2020

Right, @akashshinde would get the /helm/manifest API ( equivalent to helm get manifest) added next sprint. It will miss 4.4 feature freeze, but would be available well before code freeze.

CC @pedjak

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2020
@debsmita1 debsmita1 force-pushed the helm-release-resources branch from 60d2b45 to c0e1b28 Compare January 24, 2020 04:21
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2020
@debsmita1 debsmita1 force-pushed the helm-release-resources branch 2 times, most recently from 1505e92 to 65599af Compare January 24, 2020 09:52
@debsmita1 debsmita1 force-pushed the helm-release-resources branch 3 times, most recently from ed55252 to d349636 Compare January 24, 2020 11:02
Copy link
Contributor

@sahil143 sahil143 left a comment

Choose a reason for hiding this comment

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

Verified changes locally
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020
@sbose78
Copy link

sbose78 commented Jan 24, 2020

@christianvogt @debsmita1 @siamaksade

@pedjak brought up a very good point in https://issues.redhat.com/browse/APPSVC-382 .
We do not need a new API endpoint. The information is already available in the manifest key of the response from /api/helm/releases?namespace=foo API in #3826

[
  {
    "name": "foo3",
    "info": {
      "first_deployed": "2020-01-22T13:59:18.293548246+01:00",
      "last_deployed": "2020-01-22T13:59:18.293548246+01:00",
      "deleted": "",
      "description": "Install complete",
      "status": "deployed",
      "notes": "1. Get the application URL by running these commands:\n  export POD_NAME=$(kubectl get pods --namespace foo -l \"app=foo3-mink\" -o jsonpath=\"{.items[0].metadata.name}\")\n  echo \"Visit http://127.0.0.1:8080 to use your application\"\n  kubectl port-forward $POD_NAME 8080:80\n"
    },
    "chart": {
      "metadata": {
        "name": "mink",
        "version": "0.1.0",
        "description": "A Helm chart for Kubernetes",
        "apiVersion": "v1"
      },
      "lock": null,
      "templates": [
        {
          "name": "templates/NOTES.txt",
          "data": "MS4gR2V0IHRoZSBhcHBsaWNhdGlvbiBVUkwgYnkgcnVubmluZyB0aGVzZSBjb21tYW5kczoKe3stIGlmIGNvbnRhaW5zICJOb2RlUG9ydCIgLlZhbHVlcy5zZXJ2aWNlLnR5cGUgfX0KICBleHBvcnQgTk9ERV9QT1JUPSQoa3ViZWN0bCBnZXQgLS1uYW1lc3BhY2Uge3sgLlJlbGVhc2UuTmFtZXNwYWNlIH19IC1vIGpzb25wYXRoPSJ7LnNwZWMucG9ydHNbMF0ubm9kZVBvcnR9IiBzZXJ2aWNlcyB7eyB0ZW1wbGF0ZSAiZnVsbG5hbWUiIC4gfX0pCiAgZXhwb3J0IE5PREVfSVA9JChrdWJlY3RsIGdldCBub2RlcyAtLW5hbWVzcGFjZSB7eyAuUmVsZWFzZS5OYW1lc3BhY2UgfX0gLW8ganNvbnBhdGg9InsuaXRlbXNbMF0uc3RhdHVzLmFkZHJlc3Nlc1swXS5hZGRyZXNzfSIpCiAgZWNobyBodHRwOi8vJE5PREVfSVA6JE5PREVfUE9SVC9sb2dpbgp7ey0gZWxzZSBpZiBjb250YWlucyAiTG9hZEJhbGFuY2VyIiAuVmFsdWVzLnNlcnZpY2UudHlwZSB9fQogICAgIE5PVEU6IEl0IG1heSB0YWtlIGEgZmV3IG1pbnV0ZXMgZm9yIHRoZSBMb2FkQmFsYW5jZXIgSVAgdG8gYmUgYXZhaWxhYmxlLgogICAgICAgICAgIFlvdSBjYW4gd2F0Y2ggdGhlIHN0YXR1cyBvZiBieSBydW5uaW5nICdrdWJlY3RsIGdldCBzdmMgLXcge3sgdGVtcGxhdGUgImZ1bGxuYW1lIiAuIH19JwogIGV4cG9ydCBTRVJWSUNFX0lQPSQoa3ViZWN0bCBnZXQgc3ZjIC0tbmFtZXNwYWNlIHt7IC5SZWxlYXNlLk5hbWVzcGFjZSB9fSB7eyB0ZW1wbGF0ZSAiZnVsbG5hbWUiIC4gfX0gLW8ganNvbnBhdGg9J3suc3RhdHVzLmxvYWRCYWxhbmNlci5pbmdyZXNzWzBdLmlwfScpCiAgZWNobyBodHRwOi8vJFNFUlZJQ0VfSVA6e3sgLlZhbHVlcy5zZXJ2aWNlLmV4dGVybmFsUG9ydCB9fQp7ey0gZWxzZSBpZiBjb250YWlucyAiQ2x1c3RlcklQIiAgLlZhbHVlcy5zZXJ2aWNlLnR5cGUgfX0KICBleHBvcnQgUE9EX05BTUU9JChrdWJlY3RsIGdldCBwb2RzIC0tbmFtZXNwYWNlIHt7IC5SZWxlYXNlLk5hbWVzcGFjZSB9fSAtbCAiYXBwPXt7IHRlbXBsYXRlICJmdWxsbmFtZSIgLiB9fSIgLW8ganNvbnBhdGg9InsuaXRlbXNbMF0ubWV0YWRhdGEubmFtZX0iKQogIGVjaG8gIlZpc2l0IGh0dHA6Ly8xMjcuMC4wLjE6ODA4MCB0byB1c2UgeW91ciBhcHBsaWNhdGlvbiIKICBrdWJlY3RsIHBvcnQtZm9yd2FyZCAkUE9EX05BTUUgODA4MDp7eyAuVmFsdWVzLnNlcnZpY2UuZXh0ZXJuYWxQb3J0IH19Cnt7LSBlbmQgfX0K"
        },
        {
          "name": "templates/_helpers.tpl",
          "data": "e3svKiB2aW06IHNldCBmaWxldHlwZT1tdXN0YWNoZTogKi99fQp7ey8qCkV4cGFuZCB0aGUgbmFtZSBvZiB0aGUgY2hhcnQuCiovfX0Ke3stIGRlZmluZSAibmFtZSIgLX19Cnt7LSBkZWZhdWx0IC5DaGFydC5OYW1lIC5WYWx1ZXMubmFtZU92ZXJyaWRlIHwgdHJ1bmMgMjQgLX19Cnt7LSBlbmQgLX19Cgp7ey8qCkNyZWF0ZSBhIGRlZmF1bHQgZnVsbHkgcXVhbGlmaWVkIGFwcCBuYW1lLgpXZSB0cnVuY2F0ZSBhdCAyNCBjaGFycyBiZWNhdXNlIHNvbWUgS3ViZXJuZXRlcyBuYW1lIGZpZWxkcyBhcmUgbGltaXRlZCB0byB0aGlzIChieSB0aGUgRE5TIG5hbWluZyBzcGVjKS4KKi99fQp7ey0gZGVmaW5lICJmdWxsbmFtZSIgLX19Cnt7LSAkbmFtZSA6PSBkZWZhdWx0IC5DaGFydC5OYW1lIC5WYWx1ZXMubmFtZU92ZXJyaWRlIC19fQp7ey0gcHJpbnRmICIlcy0lcyIgLlJlbGVhc2UuTmFtZSAkbmFtZSB8IHRydW5jIDI0IC19fQp7ey0gZW5kIC19fQo="
        },
        {
          "name": "templates/deployment.yaml",
          "data": "YXBpVmVyc2lvbjogZXh0ZW5zaW9ucy92MWJldGExCmtpbmQ6IERlcGxveW1lbnQKbWV0YWRhdGE6CiAgbmFtZToge3sgdGVtcGxhdGUgImZ1bGxuYW1lIiAuIH19CiAgbGFiZWxzOgogICAgY2hhcnQ6ICJ7eyAuQ2hhcnQuTmFtZSB9fS17eyAuQ2hhcnQuVmVyc2lvbiB9fSIKc3BlYzoKICByZXBsaWNhczoge3sgLlZhbHVlcy5yZXBsaWNhQ291bnQgfX0KICB0ZW1wbGF0ZToKICAgIG1ldGFkYXRhOgogICAgICBsYWJlbHM6CiAgICAgICAgYXBwOiB7eyB0ZW1wbGF0ZSAiZnVsbG5hbWUiIC4gfX0KICAgIHNwZWM6CiAgICAgIGNvbnRhaW5lcnM6CiAgICAgIC0gbmFtZToge3sgLkNoYXJ0Lk5hbWUgfX0KICAgICAgICBpbWFnZTogInt7IC5WYWx1ZXMuaW1hZ2UucmVwb3NpdG9yeSB9fTp7eyAuVmFsdWVzLmltYWdlLnRhZyB9fSIKICAgICAgICBpbWFnZVB1bGxQb2xpY3k6IHt7IC5WYWx1ZXMuaW1hZ2UucHVsbFBvbGljeSB9fQogICAgICAgIHBvcnRzOgogICAgICAgIC0gY29udGFpbmVyUG9ydDoge3sgLlZhbHVlcy5zZXJ2aWNlLmludGVybmFsUG9ydCB9fQogICAgICAgIGxpdmVuZXNzUHJvYmU6CiAgICAgICAgICBodHRwR2V0OgogICAgICAgICAgICBwYXRoOiAvCiAgICAgICAgICAgIHBvcnQ6IHt7IC5WYWx1ZXMuc2VydmljZS5pbnRlcm5hbFBvcnQgfX0KICAgICAgICByZWFkaW5lc3NQcm9iZToKICAgICAgICAgIGh0dHBHZXQ6CiAgICAgICAgICAgIHBhdGg6IC8KICAgICAgICAgICAgcG9ydDoge3sgLlZhbHVlcy5zZXJ2aWNlLmludGVybmFsUG9ydCB9fQogICAgICAgIHJlc291cmNlczoKe3sgdG9ZYW1sIC5WYWx1ZXMucmVzb3VyY2VzIHwgaW5kZW50IDEyIH19Cg=="
        },
        {
          "name": "templates/service.yaml",
          "data": "YXBpVmVyc2lvbjogdjEKa2luZDogU2VydmljZQptZXRhZGF0YToKICBuYW1lOiB7eyB0ZW1wbGF0ZSAiZnVsbG5hbWUiIC4gfX0KICBsYWJlbHM6CiAgICBjaGFydDogInt7IC5DaGFydC5OYW1lIH19LXt7IC5DaGFydC5WZXJzaW9uIH19IgpzcGVjOgogIHR5cGU6IHt7IC5WYWx1ZXMuc2VydmljZS50eXBlIH19CiAgcG9ydHM6CiAgLSBwb3J0OiB7eyAuVmFsdWVzLnNlcnZpY2UuZXh0ZXJuYWxQb3J0IH19CiAgICB0YXJnZXRQb3J0OiB7eyAuVmFsdWVzLnNlcnZpY2UuaW50ZXJuYWxQb3J0IH19CiAgICBwcm90b2NvbDogVENQCiAgICBuYW1lOiB7eyAuVmFsdWVzLnNlcnZpY2UubmFtZSB9fQogIHNlbGVjdG9yOgogICAgYXBwOiB7eyB0ZW1wbGF0ZSAiZnVsbG5hbWUiIC4gfX0K"
        }
      ],
      "values": {
        "image": {
          "pullPolicy": "IfNotPresent",
          "repository": "nginx",
          "tag": "stable"
        },
        "replicaCount": 1,
        "resources": {
          "limits": {
            "cpu": "100m",
            "memory": "128Mi"
          },
          "requests": {
            "cpu": "100m",
            "memory": "128Mi"
          }
        },
        "service": {
          "externalPort": 80,
          "internalPort": 80,
          "name": "nginx",
          "type": "ClusterIP"
        }
      },
      "schema": null,
      "files": [
        {
          "name": ".helmignore",
          "data": "IyBQYXR0ZXJucyB0byBpZ25vcmUgd2hlbiBidWlsZGluZyBwYWNrYWdlcy4KIyBUaGlzIHN1cHBvcnRzIHNoZWxsIGdsb2IgbWF0Y2hpbmcsIHJlbGF0aXZlIHBhdGggbWF0Y2hpbmcsIGFuZAojIG5lZ2F0aW9uIChwcmVmaXhlZCB3aXRoICEpLiBPbmx5IG9uZSBwYXR0ZXJuIHBlciBsaW5lLgouRFNfU3RvcmUKIyBDb21tb24gVkNTIGRpcnMKLmdpdC8KLmdpdGlnbm9yZQouYnpyLwouYnpyaWdub3JlCi5oZy8KLmhnaWdub3JlCi5zdm4vCiMgQ29tbW9uIGJhY2t1cCBmaWxlcwoqLnN3cAoqLmJhawoqLnRtcAoqfgojIFZhcmlvdXMgSURFcwoucHJvamVjdAouaWRlYS8KKi50bXByb2oK"
        }
      ]
    },
    "manifest": "---\n# Source: mink/templates/service.yaml\napiVersion: v1\nkind: Service\nmetadata:\n  name: foo3-mink\n  labels:\n    chart: \"mink-0.1.0\"\nspec:\n  type: ClusterIP\n  ports:\n  - port: 80\n    targetPort: 80\n    protocol: TCP\n    name: nginx\n  selector:\n    app: foo3-mink\n---\n# Source: mink/templates/deployment.yaml\napiVersion: extensions/v1beta1\nkind: Deployment\nmetadata:\n  name: foo3-mink\n  labels:\n    chart: \"mink-0.1.0\"\nspec:\n  replicas: 1\n  template:\n    metadata:\n      labels:\n        app: foo3-mink\n    spec:\n      containers:\n      - name: mink\n        image: \"nginx:stable\"\n        imagePullPolicy: IfNotPresent\n        ports:\n        - containerPort: 80\n        livenessProbe:\n          httpGet:\n            path: /\n            port: 80\n        readinessProbe:\n          httpGet:\n            path: /\n            port: 80\n        resources:\n            limits:\n              cpu: 100m\n              memory: 128Mi\n            requests:\n              cpu: 100m\n              memory: 128Mi\n",
    "version": 1,
    "namespace": "foo"
  }
]

You would have to query the actual status of the resources before you show them on the right panel, anyway.

@debsmita1 debsmita1 force-pushed the helm-release-resources branch from d349636 to 002255a Compare January 24, 2020 14:23
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020
@sahil143
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020
@sbose78
Copy link

sbose78 commented Jan 24, 2020

Are we using the helm backend API?

@debsmita1
Copy link
Contributor Author

Are we using the helm backend API?

@sbose78 in this PR ? No. I am currently fetching the resources using the label release=<release name>

@christianvogt
Copy link
Contributor

@serenamarie125 i think we should revisit these filters as well when we fix the resource list.

@christianvogt
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, debsmita1, sahil143

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2020
@debsmita1
Copy link
Contributor Author

/test e2e-gcp-console

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@spadgett
Copy link
Member

/hold
for merge queue fix #4065

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 24, 2020
@spadgett
Copy link
Member

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 25, 2020
@openshift-merge-robot openshift-merge-robot merged commit 0987a3b into openshift:master Jan 25, 2020
@spadgett spadgett added this to the v4.4 milestone Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dev-console Related to dev-console kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants