Reinstate webhook tests #1072
Reinstate webhook tests #1072
Conversation
rajivnathan
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
ranakan19 and
mfrancisc
as code owners
|
| @@ -2296,11 +2295,6 @@ func (a *MemberAwaitility) verifySecret(t *testing.T) []byte { | |||
| } | |||
|
|
|||
| func (a *MemberAwaitility) verifyMutatingWebhookConfig(t *testing.T, ca []byte) { | |||
| if val := os.Getenv("skip-webhook-checks-on-setup"); val == "true" { | |||
There was a problem hiding this comment.
I guess we can keep these checks for the future use and just remove the t.Setenv("skip-webhook-checks-on-setup", "true") above?
| // set env var to skip the mutating webhook check on migration setup temporarily since the old deployment | ||
| // will deploy the webhooks with the old configuration but the tests will be expecting the new configuration | ||
| // This should be removed after PR https://github.com/codeready-toolchain/toolchain-e2e/pull/809 is merged | ||
| t.Setenv("skip-webhook-checks-on-setup", "true") |
There was a problem hiding this comment.
in addition to Alexey's comment maybe we could keep this commented so that we can just uncomment when needed. WDYT? Of course adjusting a bit the description above.
There was a problem hiding this comment.
I'm sorry, but why did we actually need this configuration option at the first place?
There was a split of the init wait logic to two functions:
- WaitForOperators (used in setup_migration_test test suite)
- WaitForDeployments (used in all other cases)
where the first one only waits until the operators are deployed, so we don't need to deal with these chicken-egg problems.
As I see it, the original PR should have worked without having this configuration hack & environment set.
There was a problem hiding this comment.
The webhook test was failing so I thought it was due to the migration thing. I forgot that we made the change to the wait logic. I guess there was something actually wrong with the webhook tests then.
There was a problem hiding this comment.
Yeah, I checked the failure in the other PR and it wasn't related to setup migration test suite.
Anyway, let's drop this and keep an eye on any flakiness/failures related to migration.
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, rajivnathan 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 |
It's the follow up to #1070 to re-enable the webhook tests