set approved to false after user sign up deactivated #801
Conversation
|
Hi @mmulholla. Thanks for your PR. I'm waiting for a codeready-toolchain member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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 kubernetes/test-infra repository. |
test/metrics/metrics_test.go
Outdated
| @@ -138,7 +138,7 @@ func TestMetricsWhenUsersManuallyApprovedAndThenDeactivated(t *testing.T) { | |||
| hostAwait.WaitForMetricDelta(t, wait.UserSignupsMetric, 2) // all signups (even if deactivated) | |||
| hostAwait.WaitForMetricDelta(t, wait.UsersPerActivationsAndDomainMetric, 2, "activations", "1", "domain", "internal") // all deactivated (but this metric is never decremented) | |||
| hostAwait.WaitForMetricDelta(t, wait.UsersPerActivationsAndDomainMetric, 0, "activations", "1", "domain", "external") // never incremented | |||
| hostAwait.WaitForMetricDelta(t, wait.UserSignupsApprovedMetric, 2) // all deactivated (but counters are never decremented) | |||
| hostAwait.WaitForMetricDelta(t, wait.UserSignupsApprovedMetric, 0) // all deactivated (but counters are never decremented) | |||
There was a problem hiding this comment.
I'm not sure the counter will be decremented
There was a problem hiding this comment.
It did pass when I ran it locally.
There was a problem hiding this comment.
That's weird. AFAIK the "approved" metric is not supposed to decrement. @xcoulon can you confirm?
There was a problem hiding this comment.
@mmulholla from the failure here: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/codeready-toolchain_toolchain-e2e/801/pull-ci-codeready-toolchain-toolchain-e2e-master-e2e/1708871838563569664/build-log.txt
it is failing on the parallel test suite, so it has not yet ran the metrics test suite.
I think you still have to update those conditions:
There was a problem hiding this comment.
@mfrancisc you were, of course, correct, I removed the change to decrement the count.
There was a problem hiding this comment.
Also updated condition in registration_service_test.
There was a problem hiding this comment.
thanks for addressing my comment!
|
/ok-to-test |
|
/ok-to-test |
mmulholla
force-pushed
the
RemoveUserSignUpApprovalAfterDeactivation
branch
from
4fbe2a0 to
c6f3e80
Compare
|
@mmulholla: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes/test-infra repository. I understand the commands that are listed here. |
mmulholla
force-pushed
the
RemoveUserSignUpApprovalAfterDeactivation
branch
from
4b11038 to
d4eba97
Compare
mmulholla
changed the title
|
/retest |
|
Kudos, SonarCloud Quality Gate passed!
|
| @@ -295,7 +295,7 @@ func TestVerificationRequiredMetric(t *testing.T) { | |||
| // when reactivating the user | |||
| InvokeEndpoint(t, "POST", route+"/api/v1/signup", token0, "", http.StatusAccepted) | |||
| userSignup, err = hostAwait.WaitForUserSignup(t, identity0.Username, | |||
| wait.UntilUserSignupHasConditions(wait.ConditionSet(wait.Default(), wait.VerificationRequired())...), | |||
| wait.UntilUserSignupHasConditions(wait.ConditionSet(wait.Default(), wait.VerificationRequired(), wait.ApprovedDeactivated())...), | |||
There was a problem hiding this comment.
This is something we should improve in the future as well. Instead of keeping the old "Deactivated' reason we could add a different one. But it's fine for now 👍
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MatousJobanek, mfrancisc, mmulholla 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 |
Working on getting e2e tests working.
re: codeready-toolchain/host-operator#868