Adds webhook for Repository Validation #708
Conversation
this add validation webhook for repository so that we dont allow user to create multiple respositories with same git url. Signed-off-by: Shivam Mukhade <smukhade@redhat.com>
Signed-off-by: Shivam Mukhade <smukhade@redhat.com>
Signed-off-by: Shivam Mukhade <smukhade@redhat.com>
|
it seems in e2e we don't cleanup repo crs π |
Signed-off-by: Shivam Mukhade <smukhade@redhat.com>
Signed-off-by: Shivam Mukhade <smukhade@redhat.com>
Signed-off-by: Shivam Mukhade <smukhade@redhat.com>
| @@ -58,6 +59,22 @@ jobs: | |||
| tags: ${{ steps.meta-watcher.outputs.tags }} | |||
| labels: ${{ steps.meta-watcher.outputs.labels }} | |||
|
|
|||
| - name: Extract metadata (tags, labels) for Docker (Webhook) | |||
There was a problem hiding this comment.
i think there is.a way to do "matrix" in google actions for not have to copy and paste large block everytime, i guess we can do that if we have to add another image again π
There was a problem hiding this comment.
we can use matrix but there will 4 jobs created currently we execute all in one
pkg/webhook/validation.go
Outdated
| } | ||
|
|
||
| func checkIfRepoExist(ctx context.Context, pac versioned.Interface, repo *v1alpha1.Repository, ns string) (bool, error) { | ||
| repositories, err := pac.PipelinesascodeV1alpha1().Repositories(ns).List( |
There was a problem hiding this comment.
I wonder if it's an expensive operation and if we should cache,
There was a problem hiding this comment.
gonna use a lister here instead of client
| runcnx.Clients.Log.Infof("Deleting NS %s", targetNS) | ||
| err := runcnx.Clients.Kube.CoreV1().Namespaces().Delete(ctx, targetNS, metav1.DeleteOptions{}) |
There was a problem hiding this comment.
is this code never ran? i think go should have bugged us out on this?
There was a problem hiding this comment.
it is ran.. but we are deleting repo before deleting ns
test were failing as repo were not getting deleted from etcd
when ns was deleted
There was a problem hiding this comment.
i mean the (old) code would get "no new variables on left sides" since we were redefining twice err
(i had to double check since it's early morning here :D
)
|
I think it should be relatively easy to add a E2E test for this feature (try to create the same repo crd and check for failure) |
Signed-off-by: Shivam Mukhade <smukhade@redhat.com>
Signed-off-by: Shivam Mukhade <smukhade@redhat.com>
as can't create multiple repo with same url as webhook will deny Signed-off-by: Shivam Mukhade <smukhade@redhat.com>
Codecov Report
@@ Coverage Diff @@
## main #708 +/- ##
==========================================
- Coverage 69.08% 68.10% -0.99%
==========================================
Files 65 68 +3
Lines 3921 4016 +95
==========================================
+ Hits 2709 2735 +26
- Misses 944 1009 +65
- Partials 268 272 +4
Continue to review full report at Codecov.
|
| @@ -12,23 +12,20 @@ import ( | |||
| ) | |||
|
|
|||
| func TestGithubPullRequest(t *testing.T) { | |||
| t.Parallel() | |||
There was a problem hiding this comment.
t.Parallel actually makes it parallel isnt it ?
There was a problem hiding this comment.
yeah but we use same git repo right
so one test creats repo
other will also try to create and fail in a different ns
| @@ -12,23 +12,20 @@ import ( | |||
| ) | |||
|
|
|||
| func TestGithubPullRequest(t *testing.T) { | |||
| t.Parallel() | |||
This adds a webhook (custom admission controller) to validate repository url and reject create request
if user is trying to create a repository with url which already exist in some other repository.
Code changes:
Adds a new deployment for webhook
The webhook has 2 controllers:
Submitter Checklist
make test lintbefore submitting a PR (ie: with pre-commit, no need to waste CPU cycle on CI