-
Notifications
You must be signed in to change notification settings - Fork 40
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.
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
Add conditions.Factory #58
Add conditions.Factory #58
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
Pull Request Test Coverage Report for Build 1219544249
💛 - Coveralls |
cf980c8
to
8ad7a7a
Compare
8ad7a7a
to
cc3ecf6
Compare
Closing this since there doesn't seem to be much interest in it. We can always re-visit/re-open if this comes up again. |
Hi @joelanford. We want to re-try implementing the upgradable condition in HCO, now when bugzilla bug #1927340 is fixed. So we'll need a fix for #50. |
@joelanford - could you please reconsider this? It will really help to have a solution for #50, to get more confidence in our implementation, because this is not easy to verify the behavior of the operator regarding the operator condition in e2e tests. |
cc3ecf6
to
aa2eb51
Compare
aa2eb51
to
b5de964
Compare
The conditions.NewCondition function reads cluster state to determine the namespace the operator is running in so that it can manipulate the correct OperatorConditions object in a cluster. However this function's dependency on a specific file existing in a specific location makes it difficult to test code that uses it. The Factory interface is introduced to enable users to provide their own implementations for building a Condition object to manage OperatorCondition resources. Users can now build a factory and inject it into code that builds new conditions. This enables code under test to use the Factory abstraction and enables operator authors to inject custom functionality needed when writing tests. Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
b5de964
to
61b77e2
Compare
/lgtm @varshaprasad96 - could you please merge this? |
@nunnatsa: changing LGTM is restricted to collaborators In response to this:
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. |
/lgtm |
The PR looks good, I have no objections in merging it. I'll wait for a day for additional reviews if any, if not merge it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approved
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: varshaprasad96 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 |
Description of the change:
Add conditions.Factory
Motivation for the change:
The conditions.NewCondition function reads cluster state to determine
the namespace the operator is running in so that it can manipulate the
correct OperatorConditions object in a cluster.
However this function's dependency on a specific file existing in a
specific location makes it difficult to test code that uses it.
The Factory struct is introduced to enable users to provide their own
implementations for looking up the name and namespace of the operator's
OperatorCondition object. Users can now build a factory and inject it into
code that builds new conditions. This enables code under test to use the
Factory abstraction and enables operator authors to inject custom
functionality needed when writing tests.
Closes #50