[LOGEXP 980] library change operator-sdk 1.2 #591
Conversation
|
/retest |
There was a problem hiding this comment.
Awesome work regarding enabling the metrics endpoint again. I would like a couple of concerns for discussion:
- Why do we need to migrate our test-suite to ginkgo?
- Using the controller-runtime
envtestpackage might disconnect our E2E test from a real API-Server IIRC, doesn't it? - Does the new testing approach ensure that CRs/Secrets are removed after each test?
In general the PR looks like to solve two things at a time, i.e. metrics and refactoring the E2E test suite. I might have missed something, but let's discuss if we can split these two changes in separate PRs.
| echo "Complete e2e olm test" | ||
| $(JUNITMERGE) $$(find $$JUNIT_REPORT_OUTPUT_DIR -iname "*.xml") > $(JUNIT_REPORT_OUTPUT_DIR)/junit.xml |
There was a problem hiding this comment.
Is this temporary or to be replaced by merge-junit target?
There was a problem hiding this comment.
This is temporary and will be removed in the final version. I added this because sometimes I forgot to remove the tmp directory after each iteration.
|
/retest |
1 similar comment
|
/retest |
Since v1.0, operatro-sdk generates the
Could you elaborate this? Or we can discuss this on google meeting.
yes, secret and CRs are cleaned up after each iteration (https://github.com/openshift/elasticsearch-operator/pull/591/files#diff-b0fe4827e62a6e2e85531c9c733b581e143e23bed137002c15ec52d450c40319R44-R66)
The reason they are combined in a single PR is that since sdk 1.0 (https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.0.0/#library-changes), BTW, the reason I upgrade to v1.2 is because of an issue of packing service into CSV in v1.1 (https://groups.google.com/g/operator-framework/c/MFS9Ezf1f7I). |
|
/retest |
1 similar comment
|
/retest |
If the E2E tests work for now without switching to ginkgo, I suggest to extract the ginkgo change in a separate PR. In general the team is still on a stand to keep ginkgo or not and we need to discuss this first before we move our suite. WDYT? |
|
/retest |
huikang
force-pushed
the
LOG-980-library-change-sdk-1.2
branch
from
1d7cd2d to
e050fe3
Compare
Hi, @periklis , I have updated the commit of the test code in sdk 1.2: the ginkgo part has been removed. |
huikang
force-pushed
the
LOG-980-library-change-sdk-1.2
branch
from
e050fe3 to
2ae856b
Compare
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
huikang
force-pushed
the
LOG-980-library-change-sdk-1.2
branch
2 times, most recently
from
ac11d72 to
12a5496
Compare
|
/retest |
huikang
force-pushed
the
LOG-980-library-change-sdk-1.2
branch
from
12a5496 to
37f7d0c
Compare
|
/test e2e-operator |
|
/retest |
test/e2e-olm/elasticsearch_test.go
Outdated
| for i := 0; i < 2; i++ { | ||
| name := fmt.Sprintf("elasticsearch-%v-cdm-%v-%d", esUUID, dataUUID, i+1) | ||
| key = types.NamespacedName{Name: name, Namespace: operatorNamespace} | ||
| esDeployment := &apps.Deployment{} | ||
| err = wait.Poll(cleanupRetryInterval, cleanupTimeout, func() (done bool, err error) { | ||
| err = k8sClient.Get(context.Background(), key, esDeployment) | ||
| if err != nil && errors.IsNotFound(err) { | ||
| return true, nil | ||
| } | ||
| fmt.Printf("Wait for cleaning up elasticsearch deployment %s\n", key.String()) | ||
| return false, nil | ||
| }) | ||
| if err != nil { | ||
| t.Error(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Thinking of this a little bit more, we don't need to wait for deployments to be deleted. The work is done by the k8s GC, since we delete the elasticsearch CR.
There was a problem hiding this comment.
Right. I changed to checking the status of the elasticsearch CR because somehow in the my local cluster the e2e failed between tests due to unchanged status after deleting the CR.
huikang
force-pushed
the
LOG-980-library-change-sdk-1.2
branch
from
64fc7e0 to
ddce3d4
Compare
openshift-ci-robot
added
the
approved
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
@jcantrill why did you place a hold on this? |
- remove pinned operator-sdk - upgrade controller-runtime to 0.6.3
- use operator-sdk v1.2.0 which resolves the problem of generating service and serviceMonitor in previous version
- Remove imported test, test/e2eutil from old operator-sdk - use k8s client from controller-runtime
huikang
force-pushed
the
LOG-980-library-change-sdk-1.2
branch
from
0c06f0d to
674b0ea
Compare
ewolinetz
added
the
tide/merge-method-squash
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alanconway, ewolinetz, huikang, periklis 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 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
5 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/refresh |
|
Manually merging as all tests passed and includes appropriate labels |
7 participants
Description
Update the metric service and e2e test after upgrading to operator-sdk v1.2
/cc @ewolinetz @alanconway
/assign @periklis
Links