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

Adds the custom detail to the InstalledPackageDetail. #3308

Merged

Conversation

absoludity
Copy link
Contributor

@absoludity absoludity commented Aug 26, 2021

Updates the InstalledPackageDetail to include custom details.

Helm plugin populates this with a helm specific message that includes the helm release revision.

Signed-off-by: Michael Nelson minelson@vmware.com

Description of the change

As noted in the missing fields comment, the dashboard needs to be able to present release versions for the rollback action.

Initially we thought we'd need to update the InstalledPackageDetail with a list of rollback versions, but this doesn't translate well to any other packaging systems (Flux allows rolling back, but not to a specific release version, just the previous one, so doesn't expose any release version, kapp_controller doesn't appear to have any concept of release revisions/versions yet).

After realising that the current dashboard simply generates the list of rollback targets based on the current revision, we just need to ensure that the helm rollback action knows the current release version.

I'd created another PR in #3302 to include the revision in the installed package identifier, but Dimitri pointed out that we could use the CustomDetail field which we'd not thought we'd need. It's suited perfectly for this: extra data that is specific to Helm.

Benefits

The dashboard can for now pull out the helm release version (though longer-term I'd suggest we support rolling back to the latest successful release version so the dashboard does not need this info at all),

Applicable issues

Additional information

IRL test:

grpcurl -plaintext -d '{"installed_package_ref": {"context": {"namespace": "default"}, "identifier": "test-apache"}}' localhost:8080  kubeappsapis.plugins.helm.packages.v1alpha1.HelmPackagesService.GetInstalledPackageDetail
{
  "installedPackageDetail": {
    "installedPackageRef": {
      "context": {
        "namespace": "default"
      },
      "identifier": "test-apache"
    },
    "pkgVersionReference": {
      "version": "8.5.4"
    },
    ...
    "customDetail": {
      "@type": "type.googleapis.com/kubeappsapis.plugins.helm.packages.v1alpha1.InstalledPackageDetailCustomDataHelm",
      "releaseRevision": 2
    }
  }
}

The helm plugin populates this with a message containing helm specifics,
in this case, the release revision.

Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

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

Great! Thanks for the changes! I'm implementing the associated changes in the UI by cherry-picking your commit.

Also, please let me jump in and commit a very minor change to ensure the docs fields are consistent with the rest.

@antgamdia
Copy link
Contributor

Some news: I'm able to get the revision number properly... but somehow the Any type is not being automatically decoded in javascript. The generated code just decodes the message according to the declared type, but it is not able to look the @type field up in the code.

As a result, I need to manually perform a decode as follows. I wonder if using Any is being really useful in our scenario or if it's better just to define a whole custom response type for each plugin... but I don't know if it could interfere in how the reflexive satisfaction of interfaces is being performed.

Anyway, not a big deal doing the manual decoding in the UI side

const revision =
          InstalledPackageDetailCustomDataHelm.decode(
            action.payload.app?.customDetail?.value as unknown as Uint8Array,
          ).releaseRevision ?? 0;

@absoludity absoludity merged commit d20467d into vmware-tanzu:master Aug 26, 2021
@absoludity absoludity deleted the add-release-revision-extra-detail branch August 26, 2021 23:53
@absoludity
Copy link
Contributor Author

Some news: I'm able to get the revision number properly... but somehow the Any type is not being automatically decoded in javascript. The generated code just decodes the message according to the declared type, but it is not able to look the @type field up in the code.

Yes, that's correct.

As a result, I need to manually perform a decode as follows.

Yes, it needs to be manual to be done in a type-safe way anyway, aiui. That is, you can't just do something like:

const helmCustomData = action.payload.app?.customDetail?.decode();

because the Any field may contain something other than helm custom data. So we need to be explicit about what we think we are decoding here (which is safer, and when we have other plugins enabled, we can use the plugin type to determine this).

I wonder if using Any is being really useful in our scenario or if it's better just to define a whole custom response type for each plugin... but I don't know if it could interfere in how the reflexive satisfaction of interfaces is being performed.

That would break the interfaces, as you guessed, so we wouldn't have the possibility of a generic interface/client to interact with packages.

Anyway, not a big deal doing the manual decoding in the UI side

Great.

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.

None yet

3 participants