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

refac: Migrate all pipelines db operation to drizzle on NextJs #239

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

alviskalev
Copy link
Contributor

closed #140

Copy link
Member

@dinmukhamedm dinmukhamedm left a comment

Choose a reason for hiding this comment

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

As a draft this looks good to me overall. Let's go ahead and test this/remove the redundant routes and methods in the backend

}

const UpdatePipelineSchema = z.object({
name: z.string().optional(),
visibility: z.enum(['PUBLIC', 'PRIVATE']).optional()
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 there is no way to update the "visibility" part from the UI. Let's remove handling it, this is sort of legacy. Sorry, I should have mentioned that in the issue description.

Comment on lines +293 to +294
export const DEFAULT_NEW_PIPELINE_VERSION_ID_STRING =
'db6d1708-9836-42f2-a3ea-732ca7709039';
Copy link
Member

Choose a reason for hiding this comment

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

nit, let's rename this to DEFAULT_NEW_PIPELINE_TEMPLATE_ID.
Two small changes:

  • clearer communication that this is a template. Yes, I gave the variable the original name and only now I realize how confusing it is.
  • no need to call it a string. The only reason it was called a string was an indication in Rust to parse it to UUID. As we don't need to do that in JS, we can drop that part

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.

2 participants