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.

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

Conditions: Map out-of-date bundleDeployment with an operator condition #134

Merged

Conversation

varshaprasad96
Copy link
Member

This PR brings in the change that sets the operator condition to Unknown when the BundleDeployment's conditions are out od date.

It("verify operator status when bundle deployment is waiting to be created", func() {
By("running reconcile")
bd.Status.ObservedGeneration = bd.GetGeneration()
Copy link
Member Author

Choose a reason for hiding this comment

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

This is hacky, but needed since we are trying to mimic >1 reconcile.

Copy link
Member

Choose a reason for hiding this comment

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

A better approach might be to have separate When blocks. Something along these lines, maybe?

When("The bundle deployment status is stale", func() {})
When("The bundle deployment status is up-to-date", func() {
	BeforeEach(func() {
		bd.Status.ObservedGeneration = bd.GetGeneration()
		By("updating the status of bundleDeployment")
		err := cl.Status().Update(ctx, bd)
		Expect(err).NotTo(HaveOccurred())
	}
})

Also, I find it easier to think about these tests not as mimicking multiple reconciles, but more like "what happens during a given Reconcile when the state of the world is X?". And then we would ideally have tests for all the various X's such that we get really good code coverage in Reconcile.

Copy link
Member Author

Choose a reason for hiding this comment

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

Made the change, just left the status update call in It blocks, so that we update the status only once after setting the right conditions.

@varshaprasad96 varshaprasad96 force-pushed the fix/stale-status branch 2 times, most recently from 91a6d6b to cc2c737 Compare March 1, 2023 18:22
controllers/operator_controller.go Outdated Show resolved Hide resolved
controllers/operator_controller.go Outdated Show resolved Hide resolved
controllers/operator_controller_test.go Outdated Show resolved Hide resolved
It("verify operator status when bundle deployment is waiting to be created", func() {
By("running reconcile")
bd.Status.ObservedGeneration = bd.GetGeneration()
Copy link
Member

Choose a reason for hiding this comment

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

A better approach might be to have separate When blocks. Something along these lines, maybe?

When("The bundle deployment status is stale", func() {})
When("The bundle deployment status is up-to-date", func() {
	BeforeEach(func() {
		bd.Status.ObservedGeneration = bd.GetGeneration()
		By("updating the status of bundleDeployment")
		err := cl.Status().Update(ctx, bd)
		Expect(err).NotTo(HaveOccurred())
	}
})

Also, I find it easier to think about these tests not as mimicking multiple reconciles, but more like "what happens during a given Reconcile when the state of the world is X?". And then we would ideally have tests for all the various X's such that we get really good code coverage in Reconcile.

controllers/operator_controller.go Outdated Show resolved Hide resolved
controllers/operator_controller_test.go Outdated Show resolved Hide resolved
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Mar 3, 2023
@openshift-ci
Copy link

openshift-ci bot commented Mar 3, 2023

New changes are detected. LGTM label has been removed.

This PR brings in the change that sets the operator condition to Unknown
when the BundleDeployment's conditions are out od date.

Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
@varshaprasad96
Copy link
Member Author

@joelanford Re-organized the test cases, please feel free to let me know if you would like to see any other changes. I can do those in follow-ups, merging this for now.

@varshaprasad96 varshaprasad96 merged commit a252719 into operator-framework:main Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants