Skip to content

CP-44477 Add Typescript bindings for XenAPI #5623

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions scripts/examples/typescript/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
**/.env
**/node_modules
package-lock.json
Copy link
Member

Choose a reason for hiding this comment

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

why are we not adding package-lock? don't we want a specific dependency list?

dist/
12 changes: 12 additions & 0 deletions scripts/examples/typescript/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
.PHONY: build clean

build:
sed -i '/version/s/1.0.0/'"$(XAPI_VERSION)"'/' package.json
npm install
Copy link
Member

Choose a reason for hiding this comment

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

should be

npm ci --omit=dev

npm run build

publish:
npm publish
Comment on lines +8 to +9
Copy link
Member

Choose a reason for hiding this comment

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

honestly don't think we'd need this. why are we adding a publish here?

Copy link
Member

Choose a reason for hiding this comment

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

futhermore, in npm you'd usually use scripts in the package.json, not Makefiles. I'd move everything there if you need it

for instance, your expanded build could be build:production under scripts

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the publish might be useful if we want to publish automatically to npm like we do for XenAPI.py on pypi?

Copy link
Member

Choose a reason for hiding this comment

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

I think the publish might be useful if we want to publish automatically to npm like we do for XenAPI.py on pypi?

Yes I agree but we're not publishing right now. Unless that's planned with this PR, in which case we need GitHub actions, and either a publishing setup on GitHub or npmjs.com.

Also for publishing I'd argue we'd need to set up a XenServer account. Let's do it properly. For instance I don't see why we have this already: https://www.npmjs.com/package/xen-api-ts


clean:
rm -rf dist/ node-modules/ package-local.json
41 changes: 41 additions & 0 deletions scripts/examples/typescript/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# A Typescript bindings for XenAPI
Copy link
Member

Choose a reason for hiding this comment

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

typo here and below

Suggested change
# A Typescript bindings for XenAPI
# Typescript bindings for XenAPI

Copy link
Contributor

@kc284 kc284 May 10, 2024

Choose a reason for hiding this comment

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

I would avoid the use of "bindings" completely, first because we don't provide real bindings and second because this is not the terminology used for the rest of the SDK. We can use "module" until we provide a full blown SDK.

A Typescript bindings for XenAPI, which is inspired by [Python XenAPI](https://xapi-project.github.io/xen-api/usage.html).

## Usage
Install lib from npm repository
```
$ npm install xen-api-ts
```

The usage of Typescript XenAPI is almost identical to the Python XenAPI, except it's asynchronous and requires async/await.
```
import { xapi_client } from "xen-api-ts";

async function main() {
const session = xapi_client(process.env.HOST_URL);
try {
await session.login_with_password(process.env.USERNAME, process.env.PASSWORD);
const hosts = await session.xenapi.host.get_all();
console.log(hosts); // Do something with the retrieved hosts
} finally {
await session.xenapi.session.logout();
}
}

main();
```

For more example usage, please find in tests folder.
```
$ git clone git@github.com:acefei/xen-api-ts.git
Copy link
Contributor

@kc284 kc284 May 10, 2024

Choose a reason for hiding this comment

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

To make things simpler for the users, we're trying to minimise the number of places where people can find xenserver-sdk/xen-api documentation and help. You may want to consider moving your examples (importing the whole git history if you want) to a new folder typescript within this repo https://github.com/xenserver/xenserver-samples which is also linked from the official XenServer developer documentation. Let me know and I'll add you to the maintainers list.

$ npm install
$ echo "HOST_URL=xxx" >> .env
$ echo "USERNAME=xxx" >> .env
$ echo "PASSWORD=xxx" >> .env
$ npm test tests/getXapiVersion.ts
Copy link
Member

Choose a reason for hiding this comment

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

isn't this npm run test

```

And please find the all of Classes and its Fields in [XenAPI Reference](https://xapi-project.github.io/xen-api)

## License
This repository is licensed under the MIT License.
Copy link
Member

Choose a reason for hiding this comment

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

you have a different license between here and the package.json

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any objections to using BSD2 like the rest of the SDK?

31 changes: 31 additions & 0 deletions scripts/examples/typescript/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
{
"name": "xen-api-ts",
"version": "1.0.0",
"description": "A typescript library for Xen API",
Copy link
Member

Choose a reason for hiding this comment

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

maybe a little more descriptive wouldn't hurt. let's tell our users this isn't an SDK but some basic bindings to connect to XenServer

Copy link
Contributor

@kc284 kc284 May 10, 2024

Choose a reason for hiding this comment

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

In fact it's not bindings either since we don't expose/bind any classes/fields/calls to the respective language concepts. Library or module sounds ok to me, but I agree that we can offer a bit more detail, like, as you said, that one can connect to XS and manage resources, pools, servers, VMs, SRs etc.

"main": "dist/XenAPI.js",
"types": "dist/XenAPI.d.ts",
"files": [
"/dist"
],
"scripts": {
"test": "node -r ts-node/register -r dotenv/config ",
"build": "tsc"
},
"keywords": [
"xenserve",
Copy link
Member

Choose a reason for hiding this comment

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

typo: xenserver

"xen-api",
"typescript"
],
"author": "Su Fei <fei.su@cloud.com>",
Copy link
Member

Choose a reason for hiding this comment

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

not sure what the expected value here is but we have CSG for all SDKs rather than individual authors. maybe let's confirm what's needed here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think individual authors used to be a past practice and at some point we started replacing them with the xapi project email address (not sure if there are left overs here and there).

"license": "GPLv2",
"devDependencies": {
"@types/node": "^20.8.9",
"axios": "^1.6.0",
"dotenv": "^16.3.1",
"ts-node": "^10.9.1",
"typescript": "^5.2.2"
},
"dependencies": {
"json-rpc-2.0": "^1.6.0"
}
}
52 changes: 52 additions & 0 deletions scripts/examples/typescript/src/XenAPI.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { JSONRPCClient } from "json-rpc-2.0";
Copy link
Member

Choose a reason for hiding this comment

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

we're missing copyright notices on these files. I'm guessing that's needed for these?


class XapiSession {
Copy link
Member

Choose a reason for hiding this comment

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

there's no docs all throughout this module. let's add some for users of our client

private client: JSONRPCClient;
private session_id: string | undefined;

constructor(hostUrl: string | undefined) {
this.client = new JSONRPCClient(async (request) => {
Copy link
Member

Choose a reason for hiding this comment

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

do we really want one client per session? I'd argue XapiSession should be taking in a client of type XenApiJsonRpcClient with some defaults.

const response = await fetch(`${hostUrl}/jsonrpc`, {
method: "POST",
headers: { "content-type": "application/json" },
body: JSON.stringify(request),
});
if (response.status === 200) {
Copy link
Member

Choose a reason for hiding this comment

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

we cannot rely on the response status in jsonrpc and xapi.

see for instance this failed task.get_result call. the call has failed, but we get a 200 status

> POST /jsonrpc HTTP/1.1
> Host: 10.71.144.44
> Content-Type: application/json
> User-Agent: insomnia/9.0.0
> Accept: */*
> Content-Length: 181

| {
| 	"id": 1,
| 	"method": "task.get_result",
| 	"jsonrpc": "2.0",
| 	"params": [
| 		"OpaqueRef:123",
| 		"OpaqueRef:123"
| 	]
| }

* TLSv1.2 (IN), TLS header, Supplemental data (23):
* Mark bundle as not supporting multiuse

< HTTP/1.1 200 OK
< content-length: 129
< connection: keep-alive
< cache-control: no-cache, no-store
< content-type: application/json
< Access-Control-Allow-Origin: *
< Access-Control-Allow-Headers: X-Requested-With


* Received 129 B chunk
* Connection #0 to host 10.71.144.44 left intact

Ideally we'd have an error type for xapi exceptions (can be a generic one), and one for HTTP errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

To add a bit to the above comment, we should check whether the response Json contains Error and if it does throw an exception containing the Message and Data, otherwise the user would have to do that for every call.

const json = await response.json();
return this.client.receive(json);
} else if (request.id !== undefined) {
return Promise.reject(new Error(response.statusText));
}
});
}

async login(method: string, args: any[] = []) {
this.session_id = await this.client.request(`session.${method}`, [...args])
return this.session_id
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be within xapi_request. you can check for method === "session.login_with_password" and use that to populate the session ID

If you're exposing a login I'd argue one should expose most procedure calls so if you want to keep it barebones I'd add it within xapi_request


async xapi_request(method: string, args: any[] = []) {
return await this.client.request(`${method}`, [this.session_id, ...args])
}
}

function xapi_proxy(obj: XapiSession, path: any[] = []): any {
Copy link
Member

Choose a reason for hiding this comment

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

in JS we would use camel case but throughout the TS bindings you're writing in snake case 👀

return new Proxy(() => {}, {
get (target, property) {
return xapi_proxy(obj, path.concat(property))
},
apply (target, self, args) {
if (path.length > 0){
if(path[path.length-1].startsWith("login")) {
return obj.login(path[path.length-1], args)
} else if (path[0].toLowerCase() == 'xenapi') {
return obj.xapi_request(path.slice(1).join('.'), args)
}
}
}
})
}

export function xapi_client(url: string|undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

since we're only exporting xapi_client why not export default. Are you planning to add more in the future?

return xapi_proxy(new XapiSession(url))
}
107 changes: 107 additions & 0 deletions scripts/examples/typescript/tests/fetchUpdates.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
import { xapi_client } from "../src/XenAPI"
import axios from 'axios'

async function get_pool_ref(session: any) {
const pool = await session.xenapi.pool.get_all()
return pool[0]
}

async function set_pool_repos(session: any, pool_ref:string, repo_refs: string[]) {
return await session.xenapi.pool.set_repositories(pool_ref, repo_refs)
}

async function create_repos(session: any) {
// Ref http://xapi-project.github.io/xen-api/classes/repository.html
const repositories: { [key: string]: any } = {
base: {
name_label: 'base',
name_description: 'base rpm repo',
binary_url: 'https://repo.ops.xenserver.com/xs8/base',
source_url: 'https://repo-src.ops.xenserver.com/xs8/base',
// base repo is not an update repo
update: false,
},
normal: {
name_label: 'normal',
name_description: 'normal rpm repo',
binary_url: 'https://repo.ops.xenserver.com/xs8/normal',
source_url: 'https://repo-src.ops.xenserver.com/xs8/normal',
update: true,
},
}

const res = Object.keys(repositories).sort().map(async repoKey => {
const repo = repositories[repoKey]
const { name_label, name_description, binary_url, source_url, update } = repo
return await session.xenapi.Repository.introduce(name_label, name_description, binary_url, source_url, update)
})

const resPromises = await Promise.all(res)
return resPromises
}

async function get_repos(session: any) {
return await session.xenapi.Repository.get_all()
}

async function remove_repos(session: any) {
Copy link
Member

Choose a reason for hiding this comment

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

you're not really using typescript throughout this. Session should be a type and you should minimise (if not eliminate) any use of any

Also in XenAPI.ts you're using any when you don't really need to

const repo_refs = await get_repos(session)
const res = repo_refs.map(async (ref:string)=> {
return await session.xenapi.Repository.forget(ref)
})
await Promise.all(res)
}

async function sync_updates(session: any, pool_ref:string) {
return await session.xenapi.pool.sync_updates(pool_ref, false, '', '')
}

async function list_updates(session: any, session_id:string) {
try {
const response = await axios.get(`${process.env.HOST_URL}/updates`, {
params: {
session_id: session_id
},
httpsAgent: {
rejectUnauthorized: false
}
})
console.log('Response Data:', response.data)
return response.data
} catch (error) {
console.error('Error:', error)
}
}

async function main() {
console.log(`Login ${process.env.HOST_URL} with ${process.env.USERNAME}`)
const session = xapi_client(process.env.HOST_URL)
const sid = await session.login_with_password(process.env.USERNAME, process.env.PASSWORD)
Comment on lines +77 to +79
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate what you're doing here but if I have to be nit picky this is not how you would test in ts

I'd use jester, add it as a dev dependency, create test files, and then make them use .env to run.

This is unorthodox and not immediately clear to consumers of the npm.

They should be under the typescript folder, and that will allow you to control them all from there

If they're not tests but examples, they should be in xenserver-samples

console.log(`Login successfully with ${sid}`)

const pool_ref = await get_pool_ref(session)
let repo_refs = await get_repos(session)
if (!repo_refs.length) {
console.log('\nCreate new rpm repos')
repo_refs = await create_repos(session)
}

console.log('\nSet enabled set of repositories',repo_refs)
await set_pool_repos(session, pool_ref, repo_refs)

console.log('\nSync updates')
await sync_updates(session, pool_ref)

console.log('\nList updates')
const updates = await list_updates(session, sid)
console.log(updates)

console.log('\nClean old rpm repos')
await set_pool_repos(session, pool_ref, [])
await remove_repos(session)

await session.xenapi.session.logout()
console.log(`\nSession Logout.`)
}

main()
28 changes: 28 additions & 0 deletions scripts/examples/typescript/tests/getXapiVersion.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { xapi_client } from "../src/XenAPI"

// Ref https://xapi-project.github.io/xen-api/
async function get_api_version(session: any) {
const pool = await session.xenapi.pool.get_all()
const host = await session.xenapi.pool.get_master(pool[0])
const major = await session.xenapi.host.get_API_version_major(host)
const minor = await session.xenapi.host.get_API_version_minor(host)
return `${major}.${minor}`
}

async function main() {
console.log(`Login ${process.env.HOST_URL} with ${process.env.USERNAME}`)
const session = xapi_client(process.env.HOST_URL)
const sid = await session.login_with_password(process.env.USERNAME, process.env.PASSWORD)
console.log(`Login successfully with ${sid}`)

const ver = await get_api_version(session)
console.log(`\nCurrent XAPI Version: ${ver}`)

const hosts = await session.xenapi.host.get_all()
console.log(`\nGet Host list:\n${hosts.join("\n")}`)

await session.xenapi.session.logout()
console.log(`\nSession Logout.`)
}

main()
19 changes: 19 additions & 0 deletions scripts/examples/typescript/tsconfig.json
Copy link
Member

Choose a reason for hiding this comment

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

since this is the first time that you're adding this library, please also add eslint so we can keep this codebase clean :)

Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"compilerOptions": {
"target": "es2016", /* Set the JavaScript language version for emitted JavaScript and include compatible library declarations. */
"module": "commonjs", /* Specify what module code is generated. */
Copy link
Member

Choose a reason for hiding this comment

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

haven't looked at all the packages you're importing but can't we use esm?

"declaration": true, /* Generate .d.ts files from TypeScript and JavaScript files in your project. */
"outDir": "./dist", /* Specify an output folder for all emitted files. */
"esModuleInterop": true, /* Emit additional JavaScript to ease support for importing CommonJS modules. This enables 'allowSyntheticDefaultImports' for type compatibility. */
"forceConsistentCasingInFileNames": true, /* Ensure that casing is correct in imports. */
"strict": true, /* Enable all strict type-checking options. */
"skipLibCheck": true /* Skip type checking all .d.ts files. */
},
/* select which files the compiler processes. */
"include": [
"src/**/*"
],
"exclude": [
"node_modules"
]
}
Loading