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.

Sign up for GitHub

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

Reinstate webhook tests #1072

Merged
merged 1 commit into from
Nov 26, 2024
Merged

Reinstate webhook tests #1072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions test/migration/setup/setup_migration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,6 @@ import (

func TestSetupMigration(t *testing.T) {
// given
// 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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

awaitilities := WaitForOperators(t)

runner := migration.SetupMigrationRunner{
Expand Down
11 changes: 0 additions & 11 deletions testsupport/wait/member.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"encoding/json"
"fmt"
"os"
"reflect"
"sort"
"strings"
Expand Down Expand Up @@ -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" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can keep these checks for the future use and just remove the t.Setenv("skip-webhook-checks-on-setup", "true") above?

// skipped temporarily only for setup migration test but applies for after migration test
// This should be removed after PR https://github.com/codeready-toolchain/toolchain-e2e/pull/1070 is merged
return
}
t.Logf("checking MutatingWebhookConfiguration")
actualMutWbhConf := &admv1.MutatingWebhookConfiguration{}
a.waitForResource(t, "", "member-operator-webhook-"+a.Namespace, actualMutWbhConf)
Expand Down Expand Up @@ -2380,11 +2374,6 @@ func (a *MemberAwaitility) verifyMutatingWebhookConfig(t *testing.T, ca []byte)
}

func (a *MemberAwaitility) verifyValidatingWebhookConfig(t *testing.T, ca []byte) {
if val := os.Getenv("skip-webhook-checks-on-setup"); val == "true" {
// skipped temporarily only for setup migration test but applies for after migration test
// This should be removed after PR https://github.com/codeready-toolchain/toolchain-e2e/pull/1070 is merged
return
}
t.Logf("checking ValidatingWebhookConfiguration '%s'", "member-operator-validating-webhook"+a.Namespace)
actualValWbhConf := &admv1.ValidatingWebhookConfiguration{}
a.waitForResource(t, "", "member-operator-validating-webhook-"+a.Namespace, actualValWbhConf)
Expand Down
Loading