-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add datalink create node #19
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
base: main
Are you sure you want to change the base?
Conversation
ewels
left a comment
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.
Really nice! Haven't tested yet, but a quick scan of the code looks great, couple of v. minor comments.
What kind of use cases do you see for this? Just thinking about the examples.
| "devDependencies": { | ||
| "nodemon": "^3.1.10" | ||
| }, | ||
| "scripts": { | ||
| "dev": "nodemon --exec 'node-red -u ./test-instance' --watch nodes --ext js" |
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.
Nice! Please could you document usage in CONTRIBUTING.md so that folks know how to use it? (and anywhere else that makes sense)
| * @param {object} options - Options object with baseUrl and workspaceId | ||
| * @returns {Promise<string|null>} The resource ID, or null if resourceName is empty | ||
| */ | ||
| async function resolveResource(RED, node, msg, resourceType, resourceName, { baseUrl, workspaceId = null } = {}) { |
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.
Do we need msg as an argument? Doesn't look like it's being used.
| /** | ||
| * Generic resource resolver for Seqera Platform API. | ||
| * Resolves a resource name to its ID by querying the appropriate endpoint. | ||
| * | ||
| * Supported resource types: | ||
| * - credentials: /credentials → credentials[].id | ||
| * - data-links: /data-links → dataLinks[].id | ||
| * - pipelines: /pipelines → pipelines[].pipelineId | ||
| * - compute-envs: /compute-envs → computeEnvs[].id | ||
| * - datasets: /datasets → datasets[].id |
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.
Super nice! I reckon we could delete a fair bit of code with some refactoring to use this, the lookup is done in quite a few places I think (doesn't have to be done in this PR but it'd be a nice follow-on).
| responseKey: "datasets", | ||
| idField: "id", | ||
| nameField: "name", | ||
| queryParams: { max: "100" }, |
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 seems a bit limiting, especially for datasets..
Could the function handle pagination do you think? I implemented pagination for datasets already here.
Ideally we would have an API endpoint to do this of course, rather than pulling thousands of dataset details just to do a simple lookup 👀
Adds a node for data-link creation and utility to resolve
resourceNametoresourceIdfor simpler HTTP request handling. The function works for credentials, pipelines, compute-envs, datasets, and data-links.Changes made:
resolveResource()functionmodule.exportsresolveResourcefrom_utils.jsUpdate
CLAUDE.mdwith guidanceAdded
nodemonconfig to auto-restart local instances for quicker development throughnpm run dev