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

Validate connector name and pipeline description through API #1242

Merged
merged 20 commits into from
Nov 29, 2023

Conversation

AdamHaffar
Copy link
Contributor

Description

Adds validation for the following config values:

  • Adds validation for Connector Names so they can't be empty strings
  • Sets a character limit of 250 for Pipeline Descriptions

Fixes #574

Quick checks:

  • I have followed the Code Guidelines.
  • There is no other pull request for the same update/change.
  • I have performed manual testing through API
  • I have made sure that the PR is of reasonable size and can be easily reviewed.

@AdamHaffar AdamHaffar requested a review from a team as a code owner October 17, 2023 14:41
Copy link
Member

@lovromazgon lovromazgon left a comment

Choose a reason for hiding this comment

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

I would move these validations at least into the orchestrator, if not into the actual services. For example, we already check that the pipeline has a name here, I'd keep these validations there. This way the check is executed even if Conduit is used as a library (without endpoints).

pkg/web/api/pipeline_v1.go Outdated Show resolved Hide resolved
pkg/web/api/connector_v1.go Outdated Show resolved Hide resolved
pkg/pipeline/service.go Outdated Show resolved Hide resolved
pkg/pipeline/service.go Outdated Show resolved Hide resolved
pkg/pipeline/service.go Outdated Show resolved Hide resolved
pkg/pipeline/service.go Outdated Show resolved Hide resolved
pkg/connector/service.go Outdated Show resolved Hide resolved
Copy link
Member

@lovromazgon lovromazgon left a comment

Choose a reason for hiding this comment

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

We're getting there, I think after these comments are resolved we should be good to go 👍

pkg/connector/service.go Outdated Show resolved Hide resolved
pkg/connector/service.go Outdated Show resolved Hide resolved
pkg/pipeline/service.go Outdated Show resolved Hide resolved
pkg/connector/service.go Outdated Show resolved Hide resolved
pkg/pipeline/service.go Show resolved Hide resolved
Copy link
Member

@lovromazgon lovromazgon left a comment

Choose a reason for hiding this comment

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

Good work! I'm approving in advance, even though I left a few more nitpicks. Thanks @AdamHaffar 👍

pkg/connector/persister_test.go Outdated Show resolved Hide resolved
pkg/connector/service.go Outdated Show resolved Hide resolved
pkg/pipeline/errors.go Outdated Show resolved Hide resolved
@AdamHaffar AdamHaffar enabled auto-merge (squash) November 29, 2023 19:37
@AdamHaffar AdamHaffar merged commit 01fafc5 into main Nov 29, 2023
3 checks passed
@AdamHaffar AdamHaffar deleted the adam/validate-connector-names-api branch November 29, 2023 19:40
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.

Connector names validated in UI but not in the API
4 participants