-
Notifications
You must be signed in to change notification settings - Fork 266
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
Initial rancher-api implementation #10144
base: master
Are you sure you want to change the base?
Conversation
159cb29
to
9da1470
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the groups you split the api into.
It looks like all of these will be available in components. Do you intend to expose any of these to the bootstraping methods of extensions?
shell/initialize/index.js
Outdated
Vue.use(rancherApiPlugin, { store }); | ||
Vue.use(clusterApiPlugin, { store }); | ||
Vue.use(shellApiPlugin, { store }); | ||
Vue.use(extensionApiPlugin, { store }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will these make the API available in the bootstraping methods of the extensions? If so, isn't there a possibility that some of the stores won't be loaded yet? We might have to make that clear or error out gracefully if that's the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make the API available to extensions after some initial testing, I don't think the types are being properly referenced in an extension.
Perhaps creating a helper function to check for the existence of the store before running any commands wouldn't be a bad idea, but I believe the plugins (extensions) are initialized on line 324 which is after both the store and prototypes are created:
if (process.client && typeof plugin === 'function') {
await plugin(app.context, inject);
}
* @returns `Promise<any>` A promise that resolves with the fetched resource(s). | ||
*/ | ||
async get(type: string, options?: ResourceFetchOptions): Promise<any> { | ||
if ( this.getSchema(type) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to inform the consumer if the schema doesn't exist instead of failing silently. I notice this pattern in other parts of this pr as well.
* | ||
* @returns `Promise<any>` A promise that resolves with the fetched resource(s). | ||
*/ | ||
async get(type: string, options?: ResourceFetchOptions): Promise<any> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not obvious to me by looking at this signature how I should retrieve all items of a resource (from looking at the code it's obvious) which I think is problematic. Maybe changing options to filter instead would make that clear but I suspect we'll want to leave it as options to support pagination in the future.
I think I'd be in favor of splitting the functionality of findAll and findMatching/find into two different methods. If there's a better idea for making the usage obvious I'd be fine with something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to emulate how running a kubectl get <resource>
would return all of a resource (sans the namespace), maybe this isn't the best approach. I figured having Intellisense that explains the design of the function and options in detail would provide clear instructions on how to use the method.
I'm open to splitting the functionality as well, this may help limit confusion with further implementation.
import { SteveResource, ResourceManageOptions } from '@shell/types/rancher-api/cluster'; | ||
import BaseClusterApi from './base-cluster-class'; | ||
|
||
export default class ClusterApi extends BaseClusterApi { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a suspicion we'll want to expose more of the methods from shell/store.index.js.
A few examples:
getCluster
getClusterId
isRancher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided not to dive too far into the weeds with implementation before we agreed on a proper plan of this API.
a5cfc9d
to
e4a9bc9
Compare
63ac60b
to
864b06d
Compare
… from generated index.d.ts
a75113d
to
23f8be8
Compare
Summary
Fixes #12705
This is the initial proposal for the rancher-api spec with a few methods on the cluster and rancher classes to interact with cluster based resources.
Initialized by adding each plugin into the
pluginDefinitions
array within theinstallInjectedPlugins
method in theshell/initialize/install-plugin.js
file.Hooks:
$cluster
$rancher
$shell
@rancher/shell
components/methods$extension
This allows for Intellisense on all methods within each class for proper definitions and arguments of each method. Also adds some basic types.
Methods added to the
$shell
hook:growl
: Dispatches a growl notification with a config to determine the:title
,message
,type
, andtimeout
modal
: Opens a modal with a given config that supplies:component
,componentProps
,resources
,modalWidth
, andcloseOnClickOutside
slideInPanel
: Opens a Slide-In Panel from the right of the screen with a given config that supplies:component
,componentProps
Notable Changes
vue-shim.d.ts
global-vue.d.ts
to include typing for.vue
filesvue-shim.d.ts
andglobal-vue.d.ts
files withintypegen.sh
vue-shim.d.ts
to thetypes/shell/index.d.ts
file, we now will add a reference to that file. Adding the full contents would cause build/typing errors.Screenshots
Growl hook
Modal hook
modal-test.mp4
Slide-In Panel hook
slide-in-test.mp4