Skip to content
This repository has been archived by the owner on Jan 19, 2023. It is now read-only.

Allow plugins to apply yaml #2775

Merged

Conversation

xtreme-vikram-yadav
Copy link
Contributor

What this PR does / why we need it:

  • Allows GO plugins to apply yaml string to create/update resources.
  • Improves consistency with JS plugins interface.

Which issue(s) this PR fixes

Special notes for your reviewer:
Please merge after #2774 is merged.

@xtreme-vikram-yadav
Copy link
Contributor Author

Using Update in api service interface to create and update seems a bit odd. This PR uses update to make the api consistent with JS plugins. It can be argued to have create and update but have similar signatures and underlying implementation which can help with clarity around developers intent.

@GuessWhoSamFoo
Copy link
Contributor

The breaking changes are a bit too aggressive imo.

Create and Update has error handling for specific cases: e.g. create when the object already exists, update on an object which does not exist

Moreover, the preferred method of fetching objects is through store.Get which returns unstructured objects. The proposed changes here mean a conversion to YAML is required for each update operation.

In theory, one can already do the conversion from YAML via something like k8s.io/client-go/kubernetes/scheme. We could provide abstractions to help with this

@GuessWhoSamFoo
Copy link
Contributor

I can see why typescript plugins have to do it this way because it is difficult to represent unstructure.Unstructured in https://github.com/vmware-tanzu/plugin-library-for-octant/blob/main/plugin/octant.d.ts#L143 , although in theory possible but hard to validate.

@xtreme-vikram-yadav
Copy link
Contributor Author

Based on Sam's comments, a new method (ApplyYaml) is introduced in the dashboard client api leaving the Create and Update methods intact avoiding breaking api changes and providing better control for specific actions. Since a string yaml can also be applied using an internal action (https://github.com/vmware-tanzu/octant/blob/71e75e12b05764588d3f0aede455d143e35a3355/internal/octant/actions.go#L25), this will be documented for users.

@@ -100,6 +100,19 @@ Here is an example of setting up your plugin to know when the current namespace
}
```

Besides having custom actions, there are pre-exsiting octant actions which are registered by octant internal modules and they can be leveraged to perform an action.
For example: `action.octant.dev/apply` (https://github.com/vmware-tanzu/octant/blob/71e75e12b05764588d3f0aede455d143e35a3355/internal/octant/actions.go#L25) can be used to create/update resources
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove the link to the internal package. We can make a const in pkg/actions for folks and reference that - https://github.com/vmware-tanzu/octant/blob/master/pkg/action/actions.go

Also I would use the above URL format (linking directly to master) even though that means links will not work on the preview site.

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 apologize I forgot to comment here. I am a bit confused about this. There are multiple actions which allow users to interact with the resources in octant/actions. Should we also move other actions to pkg/actions as well as part of this documentation? Or may be the path forward is to have make needed functionality available through plugin API with some duplication?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think there is some transition cruft here and that we want to expose actions for users to consume via the pkg vs. internal

- Document actions which have similar function

Signed-off-by: Vikram Yadav <yvikram@vmware.com>
@GuessWhoSamFoo
Copy link
Contributor

Some notes:

res, err := request.DashboardClient.ApplyYAML(request.Context(), "default", "apiVersion: v1\nkind: Pod\nmetadata:\n  name: static-web\n  labels:\n    role: myrole\nspec:\n  containers:\n    - name: web\n      image: nginx\n      ports:\n        - name: web\n          containerPort: 80\n          protocol: TCP\n")
if err != nil {
	return err
}

This method of triggering an action requires binding to a component and an understanding of what payload metadata is needed since there is no RunAction rpc call

button := component.NewButton("Create pod", action.Payload{"action": action.ActionApplyYaml, "namespace": "default", "update": "apiVersion: v1\nkind: Pod\nmetadata:\n  name: static-web\n  labels:\n    role: myrole\nspec:\n  containers:\n    - name: web\n      image: nginx\n      ports:\n        - name: web\n          containerPort: 80\n          protocol: TCP\n"})

ApplyYAML returns a slice of strings that seems to be a tokenized message of the server response (e.g. [Updated Pod (v1) static-web in default]) although not documented anywhere

@GuessWhoSamFoo GuessWhoSamFoo merged commit 3beb33f into vmware-archive:master Aug 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow plugins to apply yaml
3 participants