Add support for viewing and migrate Tiller releases #330

andresmgot · 2018-05-25T15:28:48Z

Requires changes in helm-crd: Add release name to CRD spec vmware-archive/helm-crd#29
Fixes Make prepending namespace to release name optional #220
Fixes Helm CRD: Ability to adopt existing Helm deployments #320

Add Tiller releases to the Applications view. Since for creating a HelmRelease from a Tiller release it is necessary to add new information (the chart repository URL and name) we have added a new form: MigrateForm.

andresmgot · 2018-05-25T15:31:20Z

dashboard/src/components/ConfirmDialog/index.tsx

-              Delete
-            </button>
-          </div>
+          {this.props.loading === true ? (


this is needed to wait until the Tiller release is deleted, if not, after clicking "Delete" it goes directly to the Applications view and the deleted application is still there.

…ate Helm CRDs

sameersbn · 2018-05-29T08:19:54Z

dashboard/src/shared/App.ts

+
+export class App {
+  public static async waitForDeletion(name: string) {
+    const timeout = 10000; // 10s


10s timeout is too short. Can we make is 30s or higher?

sameersbn · 2018-05-29T08:32:00Z

dashboard/src/components/AppView/AppControls.tsx

-        <button className="button button-danger" onClick={this.openModel}>
-          Delete
+        <button className="button" onClick={this.handleMigrateClick}>
+          Migrate


should we call this button Import App?

I set this back to Migrate since there is now a banner that explains it in detail

sameersbn · 2018-05-29T08:38:00Z

I encountered a major issue while testing this PR. I deployed the todos chart from /~https://github.com/bitnami-labs/redisdemo using the helm cli and attempted to migrate it to kubeapps. During the migration I selected stable as the repo (chart named todos DOES NOT EXIST in the stable repo). Kubeapps showed me an error and now the Applications tab shows me the Sorry! Something went wrong. message

andresmgot · 2018-05-29T08:45:22Z

Right, I can do a validation to ensure that the chart and version are present in the repository chosen. There is still a chance that for example wordpress-v1.0.0 exists in two different repositories and the migration results in a different chart though. The user should be aware of that.

prydonius

Great work on this @andresmgot.

Some thoughts about the UX. A button that just says "Import App"/"Migrate" isn't clear enough. I suggest adding a banner above the button to explain that the app is "unmanaged" and that importing it will allow you to manage (update/delete) it from Kubeapps.

I also suggest adding something to the cards in the AppList screen to indicate that the App is not fully imported. Perhaps an warning triangle icon or something could be useful here.

prydonius · 2018-05-29T22:22:51Z

dashboard/src/components/MigrateForm/MigrateForm.tsx

+    const roles = RequiredRBACRoles;
+    roles[0].verbs = ["create"];
+    switch (error && error.constructor) {
+      case MissingChart:


could we take people back to the migration form to fix this error?

that's the current behavior, if the chart exists the migration form print this error.

Ah okay, missed that - thanks!

prydonius · 2018-05-29T22:53:27Z

dashboard/src/components/DeploymentForm/DeploymentForm.tsx

+                />
+              </div>
+              <div>
+                <label htmlFor="tillerReleaseName">Release Name (Global)</label>


I think this will be confusing to users. IMO it would be better to take this opportunity to simplify the naming relationship between HelmRelease CRD and Tiller release name and just assume that both should be globally unique.

We could:

Verify that no Tiller release name exists by checking the configmaps in the kubeapps namespace

Make HelmRelease CRD un-namespaced so that it follows the same global name requirement as Tiller

Yeah, I agree it could be confusing. I would do 1. (or any other method that checks if a release already exists) more than 2. since this is something more specific to kubeapps.

Yeah, I think you're right, it doesn't make sense to impose the restriction onto Helm CRD itself.

prydonius · 2018-05-29T23:26:08Z

dashboard/src/components/MigrateForm/MigrateForm.tsx

+                <p>
+                  {" "}
+                  * If the repository containing this chart is not in the list add it{" "}
+                  <a href="/config/repos"> here </a>{" "}


add a full stop after.

We may want to make this an actual alert, or make this nicer with CSS

it's quite visible right now but let me know if you have a specific suggestion

andresmgot · 2018-05-30T07:45:45Z

Thank you for the review.

Some thoughts about the UX. A button that just says "Import App"/"Migrate" isn't clear enough. I suggest adding a banner above the button to explain that the app is "unmanaged" and that importing it will allow you to manage (update/delete) it from Kubeapps.

I can add that.

I also suggest adding something to the cards in the AppList screen to indicate that the App is not fully imported. Perhaps an warning triangle icon or something could be useful here.

I thought about that but I think it is not necessary, a warning would mean that something is wrong with the application or something that needs to be fixed. I don't think that's the case.

prydonius · 2018-05-30T23:50:48Z

dashboard/src/shared/HelmRelease.ts


    // Go through all HelmReleaseConfigMaps and parse as IApp objects
-    const apps = Object.keys(cms).map(key => this.parseRelease(helmReleaseMap[key], cms[key]));
+    const apps = Object.keys(cms).map(key => this.parseRelease(cms[key], helmReleaseMap[key]));


We need to filter this list of apps by the namespace (if provided) using the namespace in the release data (app.data.namespace). Currently we are always showing all Tiller releases despite what namespace is selected. This also has the strange side-effect that a "migrated" app or one installed from the dashboard looks like an un-migrated app (missing HelmRelease CRD) when in a different namespace.

For example, in the default namespace, Redis looks like it hasn't been migrated (icon is missing).

In the "osba" namespace, it is correct:

With this fix, "redis" and "osba" should not appear at all when in the default namespace, and likewise "default-catalog" and "modest-shrimp" should not appear in the "osba" namespace.

good catch!

prydonius · 2018-05-31T00:46:11Z

dashboard/src/components/AppView/AppView.tsx

+        <div className="container container-small text-c">
+          <p className="margin-t-small">
+            This release is not being managed by Kubeapps. To be able to upgrade or delete this
+            release <b> click in the button "Migrate" below </b>.


please use strong instead of b. Also, there is extra spacing on either side of the text that you should remove.

click in the button "Migrate" below -> click on the "Migrate" button below

prydonius · 2018-05-31T17:54:15Z

dashboard/src/components/MigrateForm/MigrateForm.tsx

+                  <a href="/config/repos"> here </a>.{" "}
+                </p>
+              </div>
+              <div>


I would remove everything below this point. None of this is configurable (and doesn't need to be for importing an app). Let's keep this form simple.

BTW, a future improvement for this form would be to search for the chart and version across all the repos and display the possible charts from the different repos it could belong too. We should create an issue for this when this gets merged.

prydonius · 2018-05-31T18:07:06Z

I thought about that but I think it is not necessary, a warning would mean that something is wrong with the application or something that needs to be fixed. I don't think that's the case.

Yes, you're right - we'd need to figure out a way to make it not appear like the app has an error. We can leave this for now and get advice from @Angelmmiguel later.

prydonius · 2018-06-04T21:47:40Z

dashboard/src/shared/HelmRelease.ts

-    return Promise.all<IApp>(apps.map(async app => this.getChart(app)));
+    const apps = await Promise.all<IApp>(releases.map(rel => this.getChart(rel)));
+
+    if (namespace) {


let's avoid some unnecessary API calls by moving this filter above the this.getChart fetching.

prydonius · 2018-06-04T21:48:26Z

dashboard/src/shared/HelmRelease.ts

+    if (namespace) {
+      // Exclude releases that are not of this namespace
+      const nsApps = [] as IApp[];
+      apps.forEach(app => {


Array.filter will be easier (no need to instantiate a new array and push) - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/filter

prydonius

One thing I noticed is that when migrating, the Tiller release gets updated two times. The controller outputs:

2018/06/05 23:54:03 Downloading repo https://kubernetes-charts.storage.googleapis.com/index.yaml index...
2018/06/05 23:54:04 Downloading https://kubernetes-charts.storage.googleapis.com/redis-3.3.5.tgz ...
2018/06/05 23:54:04 Updating release inky-orangutan
2018/06/05 23:54:04 Installed/updated release inky-orangutan
2018/06/05 23:54:04 Release status: DEPLOYED
2018/06/05 23:54:04 Downloading repo https://kubernetes-charts.storage.googleapis.com/index.yaml index...
2018/06/05 23:54:05 Downloading https://kubernetes-charts.storage.googleapis.com/redis-3.3.5.tgz ...
2018/06/05 23:54:05 Updating release inky-orangutan
2018/06/05 23:54:05 Installed/updated release inky-orangutan
2018/06/05 23:54:05 Release status: DEPLOYED

For whatever reason, it's upgrading the Tiller release twice even though the HelmRelease is only created once. Might be a small bug in the controller, I've created #345 to track this.

This lgtm, please feel free to merge.

andresmgot · 2018-06-06T08:32:10Z

Thanks @sameersbn and @prydonius! I replied in the issue you have opened.

Andres added 2 commits May 24, 2018 17:00

Make the release name configurable

9e1fcc4

Use correct release name when upgrading

a5bfdaa

andresmgot commented May 25, 2018

View reviewed changes

Add Tiller releases to Applications view. Add migration form to gener…

4d5784a

…ate Helm CRDs

andresmgot force-pushed the migrateHelmCRD branch from dccdfbe to 4d5784a Compare May 25, 2018 15:38

sameersbn mentioned this pull request May 28, 2018

Make the release name configurable #328

Closed

Rename helmCRDReleaseName and tillerReleaseName

d57cef0

sameersbn suggested changes May 29, 2018

View reviewed changes

Catch error when chart doesn't exist

7a2947f

prydonius reviewed May 29, 2018

View reviewed changes

Andres added 2 commits May 30, 2018 17:50

Avoid using helm and tiller release names

1c70efb

Minor review

128e8b6

This was referenced May 30, 2018

Helm CRD: Ability to adopt existing Helm deployments #320

Closed

Make prepending namespace to release name optional #220

Closed

Bump helm-crd version

e569831

prydonius reviewed May 30, 2018

View reviewed changes

prydonius reviewed May 31, 2018

View reviewed changes

prydonius mentioned this pull request Jun 1, 2018

Helm CRD as optional item (Proposal) #337

Closed

Apply review

f86ab83

prydonius reviewed Jun 4, 2018

View reviewed changes

Andres added 2 commits June 5, 2018 11:52

Simplify code

2a5b566

Verify app namespace

85976eb

sameersbn approved these changes Jun 5, 2018

View reviewed changes

prydonius approved these changes Jun 5, 2018

View reviewed changes

prydonius mentioned this pull request Jun 6, 2018

Tiller release gets updated twice when migrating to Kubeapps #345

Closed

andresmgot merged commit b1c599a into vmware-tanzu:master Jun 6, 2018

prydonius mentioned this pull request Jul 3, 2018

Feature request: manage non-kubeapps helm charts #214

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for viewing and migrate Tiller releases #330

Add support for viewing and migrate Tiller releases #330

andresmgot commented May 25, 2018 •

edited

Loading

andresmgot May 25, 2018 •

edited

Loading

sameersbn May 29, 2018

sameersbn May 29, 2018

andresmgot May 30, 2018

sameersbn commented May 29, 2018

andresmgot commented May 29, 2018

prydonius left a comment

prydonius May 29, 2018

andresmgot May 30, 2018

prydonius May 30, 2018

prydonius May 29, 2018

andresmgot May 30, 2018

prydonius May 30, 2018

prydonius May 29, 2018

andresmgot May 30, 2018

andresmgot commented May 30, 2018

prydonius May 30, 2018

andresmgot Jun 4, 2018

prydonius May 31, 2018

prydonius May 31, 2018

prydonius commented May 31, 2018

prydonius Jun 4, 2018

prydonius Jun 4, 2018

prydonius left a comment •

edited

Loading

andresmgot commented Jun 6, 2018

Add support for viewing and migrate Tiller releases #330

Add support for viewing and migrate Tiller releases #330

Conversation

andresmgot commented May 25, 2018 • edited Loading

andresmgot May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameersbn commented May 29, 2018

andresmgot commented May 29, 2018

prydonius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydonius commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydonius left a comment • edited Loading

Choose a reason for hiding this comment

andresmgot commented Jun 6, 2018

andresmgot commented May 25, 2018 •

edited

Loading

andresmgot May 25, 2018 •

edited

Loading

prydonius left a comment •

edited

Loading