-
Notifications
You must be signed in to change notification settings - Fork 31
(HOLD) OPRUN-3941: Add webhook tests with enhancements and fixes #430
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
(HOLD) OPRUN-3941: Add webhook tests with enhancements and fixes #430
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: camilamacedo86 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
158497e
to
44e43f7
Compare
44e43f7
to
9385026
Compare
@camilamacedo86: This pull request references OPRUN-3941 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.20.0" version, but no target version was set. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Group: "webhook.operators.coreos.io", | ||
Version: "v2", | ||
Resource: "webhooktests", | ||
} | ||
|
||
func newWebhookTestV1(name, namespace string, valid bool) *unstructured.Unstructured { | ||
func newWebhookTest(name, namespace string, valid bool) *unstructured.Unstructured { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this is not anything major at all, it was just bothering me that this function returns a webhook, so that we can use that in our tests, so newTestWebhook
seems more of an appropriate name, than newWebhookTest
. newWebhookTest
means this is a test itself, which it is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the common standard is New<StructName|Kind|Class>
In this case, the kind inside of the sample/project that we use to do the test is "kind": "WebhookTest",
so, therefore, make sense newWebhookTest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are asking for a NEW struct of WebhookTest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt.Fprintf(GinkgoWriter, "\n[pod-logs] namespace=%s\n", namespace) | ||
|
||
By("Getting all pods in the namespace") | ||
namesOut, err := RunK8sCommand(ctx, "get", "pods", "-n", namespace, "-o", "name") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a cli tool (kubectl/oc) to get pod logs? Can't we just use controller runtime client to get all the pods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QE tests will need to use the CLI so here we add the helper already for them
And in this case is more cleaner the code with oc/kubectl then with the API
I mean, the code is KISS. Otherwise, we need add more extra deps
9385026
to
8e962e9
Compare
/test openshift-e2e-aws |
1 similar comment
/test openshift-e2e-aws |
8e962e9
to
96c2e91
Compare
96c2e91
to
3d34d5a
Compare
3d34d5a
to
e25dccd
Compare
@camilamacedo86: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/84478cd0-79f7-11f0-868c-f17f1c5c9461-0 |
Test #436 is a cert fix for our upstream-e2e, and has nothing to do with OTE, so I'm not sure why you're referencing it? |
/hold |
Because the test that we faced issues in Sippy is Therefore, I wanted to test it with your PR. |
a4fe57d
to
57ab057
Compare
/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial 10 |
@camilamacedo86: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9db3f980-7a77-11f0-9317-77fddb6f96d1-0 |
57ab057
to
60f247f
Compare
/test openshift-e2e-aws |
/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial 10 (we need check if after the cert-fix it is either fixed) |
@camilamacedo86: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/389f9120-7bf9-11f0-8801-38b19603745e-0 |
60f247f
to
fa0cb6e
Compare
… certificate rotation This change is a refactor of code from openshift/origin#30059. Assisted-by: Gemini
fa0cb6e
to
b736e1b
Compare
@camilamacedo86: This pull request references Jira Issue OCPBUGS-60564, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
@camilamacedo86: This pull request references OPRUN-3941 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Closing I will open a new PR to faciliate the review and analise with the test that we do not add due the issues faced, more info: https://redhat-internal.slack.com/archives/C06KP34REFJ/p1755607157127779?thread_ts=1755602572.169319&cid=C06KP34REFJ |
From: #434
- Skips the test that is failing. We will fix it in a follow-up. We need #436 in the payload.
- Add dumping of container logs and
kubectl describe pods
output for better diagnostics.- Include targeted certificate details dump (
tls.crt
parse) when failures occur.- Add additional check to verify webhook responsiveness after certificate rotation.
This change is a refactor with fixes of the code from openshift/origin#30059.
/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial 10
Testhing with payload and GCP to ensure that the error: https://prow.ci.openshift.org/view/gs/test-platform-results/logs/periodic-ci-shiftstack-ci-release-4.20-techpreview-e2e-openstack-ovn-serial-techpreview/1955816149002227712
Will not be faced