test: Use ExpectSingletonReconciled from operatorpkg #1663
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jeevanpuchakay 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 |
k8s-ci-robot
added
cncf-cla: yes
|
Hi @jeevanpuchakay. Thanks for your PR. I'm waiting for a kubernetes-sigs 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
k8s-ci-robot
added
the
do-not-merge/work-in-progress
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
jeevanpuchakay
force-pushed
the
main
branch
from
6a57347
to
bd27373
Compare
k8s-ci-robot
added
the
do-not-merge/work-in-progress
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
presubmit is failing with, dont think it has got to do anything with the changes in this PR (reviewers please help). See similar failure in one of the open PR #1625 |
| @@ -47,6 +47,7 @@ import ( | |||
| "sigs.k8s.io/karpenter/pkg/scheduling" | |||
| "sigs.k8s.io/karpenter/pkg/test" | |||
| . "sigs.k8s.io/karpenter/pkg/test/expectations" | |||
| expectations "github.com/awslabs/operatorpkg/test/expectations" | |||
There was a problem hiding this comment.
We should ideally be able to import this as a dot import, and port whatever expectations are in the sigs.k8s.io/karpenter package into operatorpkg. Thoughts on waiting for that to be fixed before merging this?
There was a problem hiding this comment.
Rather, do you mind removing the function here in sigs/karpenter, and just use the operatorpkg one instead without needing a named import?
There was a problem hiding this comment.
Sure, do you have any thoughts about importing specific functions from a different go module? This will improve readability into where exactly a function is imported from, also if a similar function is defined in multiple dependency modules it may lead to conflicts (not sure if go does handle it already during compile time). This way our imports will be humongous.
There was a problem hiding this comment.
Infact I am running into this methods conflict issue when I am trying to import . and use the methods from operatorpkg where other non-migrated expectations are conflicting.
# sigs.k8s.io/karpenter/pkg/controllers/disruption_test [sigs.k8s.io/karpenter/pkg/controllers/disruption.test]
/mnt/c/Users/jeeva/Documents/GitHub/karpenter/pkg/controllers/disruption/consolidation_test.go:50:2: ExpectObjectReconciled redeclared in this block
/mnt/c/Users/jeeva/Documents/GitHub/karpenter/pkg/test/expectations/expectations.go:179:6: other declaration of ExpectObjectReconciled
/mnt/c/Users/jeeva/Documents/GitHub/karpenter/pkg/controllers/disruption/consolidation_test.go:50:2: ExpectDeleted redeclared in this block
/mnt/c/Users/jeeva/Documents/GitHub/karpenter/pkg/test/expectations/expectations.go:162:6: other declaration of ExpectDeleted
.....
.....
More errors
There was a problem hiding this comment.
I would prefer we do this migration one expectation at a time (I am new to go and in general to kubernetes/karpenter) since migrating all at once may invite bunch of issues and it would be uphill battle to solve all at once. Open to hear your thoughts as well.
jeevanpuchakay
force-pushed
the
main
branch
from
74a1d5a
to
bd27373
Compare
jeevanpuchakay
force-pushed
the
main
branch
from
bd27373
to
f8cf32a
Compare
Fixes 1348
Description
operatorpkgso that we can avoid duplication at multiple places.ExpectSingletonReconciledas part of this PR to align the overall change with reviewers before creating a bigger PR.Issue tracker
How was this change tested?
make testlocally.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.