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

Validating webhook implementation #274

Merged
merged 14 commits into from
Aug 16, 2024
Merged

Conversation

ichbinblau
Copy link
Contributor

@ichbinblau ichbinblau commented Aug 7, 2024

Description

The PR implements a validating webhook to GMC which makes the following validations:

  1. Root node existence
  2. StepName validation
  3. nodeName existence
  4. serviceName uniqueness

Issues

List the issue or RFC link this PR is working on. If there is no such link, please mark it as n/a.

Type of change

List the type of change like below. Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • [X ] New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would break existing design and interface)

Dependencies

N/A

Tests

Unit test: 8096d89
E2E test: ed34944

@ichbinblau ichbinblau marked this pull request as draft August 7, 2024 04:58
@ichbinblau ichbinblau force-pushed the webhook branch 2 times, most recently from 42e4ad4 to 06d9e81 Compare August 7, 2024 06:06
lianhao
lianhao previously requested changes Aug 8, 2024
microservices-connector/helm/templates/service.yaml Outdated Show resolved Hide resolved
microservices-connector/helm/values.yaml Outdated Show resolved Hide resolved
microservices-connector/helm/templates/deployment.yaml Outdated Show resolved Hide resolved
.github/workflows/scripts/e2e/gmc_install.sh Show resolved Hide resolved
@ichbinblau ichbinblau force-pushed the webhook branch 2 times, most recently from 719ff97 to 23db7a3 Compare August 8, 2024 05:48
Copy link
Collaborator

@lianhao lianhao left a comment

Choose a reason for hiding this comment

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

helm part (and only helm part) LGTM

@lianhao lianhao self-requested a review August 8, 2024 06:32
@lianhao lianhao dismissed their stale review August 8, 2024 06:33

all have been resolved

@ichbinblau
Copy link
Contributor Author

helm part (and only helm part) LGTM

Thanks for the review and the help on troubleshooting!

Copy link
Collaborator

@zhlsunshine zhlsunshine left a comment

Choose a reason for hiding this comment

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

LGTM

}
})
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a use case for ENDPOINT?
for example,

      - name: Embedding
        internalService:
          serviceName: embedding-svc
          config:
            endpoint: /v1/embeddings
            TEI_EMBEDDING_ENDPOINT: tei-embedding-svc

in this step, TEI_EMBEDDING_ENDPOINT: tei-embedding-svc means there must be a step with service name "tei-embedding-svc" in this node.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree this is a good validation case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I try to implement it if time permits.

@KfreeZ
Copy link
Collaborator

KfreeZ commented Aug 12, 2024

thanks for contributing @ichbinblau I left a comment but I think it can be added in next PR.
@irisdingbj I think unit tests are good enough for validator to cover the feature, e2e tests for validator are not necessary.

@irisdingbj
Copy link
Collaborator

overall LGTM, for e2e, we need add one case, like invalid gmc config has provided and the validaitonwebhook will check and reject the invalid config

@ichbinblau
Copy link
Contributor Author

overall LGTM, for e2e, we need add one case, like invalid gmc config has provided and the validaitonwebhook will check and reject the invalid config

Hi, @irisdingbj where shall I put the e2e test? In the cicd script or in the test/e2e folder based on ginkgo?

@irisdingbj
Copy link
Collaborator

irisdingbj commented Aug 13, 2024

Signed-off-by: ichbinblau <[email protected]>
@irisdingbj irisdingbj marked this pull request as ready for review August 15, 2024 16:15
Copy link
Collaborator

@irisdingbj irisdingbj left a comment

Choose a reason for hiding this comment

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

@ichbinblau please resolve conflict and we can get the PR in

Copy link
Collaborator

@KfreeZ KfreeZ left a comment

Choose a reason for hiding this comment

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

thanks for contributing this !!!

@ichbinblau ichbinblau force-pushed the webhook branch 3 times, most recently from fd9bd1d to fbee849 Compare August 16, 2024 04:07
@KfreeZ KfreeZ merged commit df5f6f3 into opea-project:main Aug 16, 2024
9 checks passed
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.

5 participants