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

Upgrade AdmissionReview e2e test image to also support v1 #81271

Merged
merged 1 commit into from Aug 15, 2019

Conversation

jpbetz
Copy link
Contributor

@jpbetz jpbetz commented Aug 11, 2019

Update webhook test image support v1beta1 and v1 AdmissionReview APIs
Arrange code so that internally, test admission handlers convert v1beta1 API requests to v1 before handling, but can easily handle versions via distinct code paths if needed in the future

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

Special notes for your reviewer:

I am unfamiliar with how to test or publish the agnhost images.

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/cc @liggitt @roycaihw @sttts @caesarxuchao
/sig api-machinery
/area admission-control

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2019
@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 11, 2019
@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. area/admission-control cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Aug 11, 2019
@jpbetz
Copy link
Contributor Author

jpbetz commented Aug 12, 2019

@BCLAU What do I need to do to test the agnhost image change in this PR?

@claudiubelu
Copy link
Contributor

@BCLAU What do I need to do to test the agnhost image change in this PR?

So, first of all, since this changes webhook, we'll have to bump the image version. bump the version in test/images/agnhost/VERSION. Next, the agnhost version will also have to be bumped in test/utils/image/manifest.go.

I don't think the image promotion automation finished yet, so someone who can build and push the image to gcr.io will have to build and push it.

/cc @spiffxp

test/images/agnhost/webhook/addlabel.go Show resolved Hide resolved
test/images/agnhost/webhook/main.go Outdated Show resolved Hide resolved
test/images/agnhost/webhook/main.go Outdated Show resolved Hide resolved
@liggitt liggitt added this to Required for GA, in progress in Admission Webhooks Aug 13, 2019
@jpbetz
Copy link
Contributor Author

jpbetz commented Aug 13, 2019

@BCLAU One more question: Is there a recipe for running e2e tests against a not-yet-published agnhost image. E.g steps to publish the image to my local docker repo and update e2e tests to use it?

@jpbetz jpbetz force-pushed the webhok-agnhost-v1 branch 2 times, most recently from 342b193 to c5eb069 Compare August 13, 2019 23:37
@jpbetz
Copy link
Contributor Author

jpbetz commented Aug 13, 2019

@BCLAU One more question: Is there a recipe for running e2e tests against a not-yet-published agnhost image. E.g steps to publish the image to my local docker repo and update e2e tests to use it?

Nevermind, I found a way to run it locally.

@jpbetz
Copy link
Contributor Author

jpbetz commented Aug 13, 2019

@BCLAU @spiffxp This PR updates the agnhost image and contains a test/images/agnhost/VERSION bump. What are next steps to have that image pushed? Can we push it before merging this PR once it is approved or will we need to update test/utils/image/manifest.go in a separate PR?

@jpbetz
Copy link
Contributor Author

jpbetz commented Aug 14, 2019

/retest

@jpbetz jpbetz changed the title [WIP] Upgrade AdmissionReview e2e test image to also support v1 Upgrade AdmissionReview e2e test image to also support v1 Aug 14, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 14, 2019
@jpbetz jpbetz force-pushed the webhok-agnhost-v1 branch 3 times, most recently from ac1c135 to 1545378 Compare August 14, 2019 07:17
@jpbetz
Copy link
Contributor Author

jpbetz commented Aug 14, 2019

/retest

@liggitt
Copy link
Member

liggitt commented Aug 14, 2019

/approve
one godoc nit, lgtm otherwise

/hold
for agn image update coordination and verification of manual testing with v1 admissionreview (xref #81271 (comment))

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 14, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2019
@liggitt
Copy link
Member

liggitt commented Aug 14, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 14, 2019
@liggitt
Copy link
Member

liggitt commented Aug 14, 2019

/retest

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 14, 2019
@liggitt
Copy link
Member

liggitt commented Aug 15, 2019

/retest
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 15, 2019
@jpbetz
Copy link
Contributor Author

jpbetz commented Aug 15, 2019

e2e are passing in my development environment with new image.

Steps to reproduce:

 cd test/images/agnhost
 export VERSION=<pick something>                                                                                                                                     
 export REPO=<personal repo, e.g. gcr.io/username-images>
 <edit Dockerfile, remove CROSS_BUILD_COPY, set FROM to "alpine:3.6">
 docker build -t ${REPO}/agnhost:${VERSION} .
 docker push ${REPO}/agnhost:${VERSION}

 <edit test/utils/image/manifest.go, replace e2eRegistry with "${REPO}" and version with "${VERSION}">
 <edit test/e2e/apimachinery/webhook.go, setting "AdmissionReviewVersions: []string{"v1", "v1beta1"}" everywhere>

 kubetest --build --up --test --test_args="--ginkgo.focus=AdmissionWebhook"

@liggitt
Copy link
Member

liggitt commented Aug 15, 2019

thanks

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 15, 2019
@k8s-ci-robot k8s-ci-robot merged commit 661e4d5 into kubernetes:master Aug 15, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 15, 2019
@liggitt liggitt moved this from Required for GA, in progress to Complete in Admission Webhooks Aug 18, 2019
@jpbetz
Copy link
Contributor Author

jpbetz commented Aug 18, 2019

@spiffxp @BCLAU we need a build and push of the agnhost image. This PR bumped the version to 2.5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/admission-control area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants