Reflect BundleUnpacking cond removal in missed e2e test

[anik120]

PR #3166 removes the BundleUnpacking condition once resolution is successful. PR #3166 modified an e2e test to reflect that change. However, the test being fixed in this PR is skipped for upstream, and runs only downstream.This PR is a follow up to #3166 to reflect the BundleUnpacking condition removal in the remaining test.

Description of the change:

Motivation for the change:

Architectural changes:

Testing remarks:

Reviewer Checklist

• Implementation matches the proposed design, or proposal is updated to match implementation
• Sufficient unit test coverage
• Sufficient end-to-end test coverage
• Bug fixes are accompanied by regression test(s)
• e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
• tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
• Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
• Docs updated or added to /doc
• Commit messages sensible and descriptive
• Tests marked as [FLAKE] are truly flaky and have an issue
• Code is properly formatted

[stevekuznetsov]

sidenote that a condition being present with conditionunknown is not the same thing as a condition being absent, the GetCondition() impl is sufficiently surprising ... we might want to clean up that semantic in the future

[anik120]

That is interesting. The more I dig into this area the more messy it looks. I'd love to create an issue so that we capture "what should ideally happen" so that we can actually clean these things up. For capturing "what should happen" in the issue, @stevekuznetsov what's the canonical way of proving condition isn't present?

[joelanford]

I suspect the upstream API conventions are the reason for that logic:

Controllers should apply their conditions to a resource the first time they visit the resource, even if the status is Unknown. This allows other components in the system to know that the condition exists and the controller is making progress on reconciling that resource.

Not all controllers will observe the previous advice about reporting "Unknown" or "False" values. For known conditions, the absence of a condition status should be interpreted the same as Unknown, and typically indicates that reconciliation has not yet finished (or that the resource state may not yet be observable).

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties

[anik120]

Ah okay. So setting the conditions as False is in fact the right way to go. Which means, in Ankita's PR, she was doing the right thing by setting the condition to false. However, "the mess" I kept uncovering was because after Ankita did the right thing for one condition, there existed a mixture of how we handle the conditions in the CR, some were being removed, while one was being set to false. And this confusion of some conditions being removed and one condition being set to False led to the original "why isn't the ResolutionFailed condition being reset even after the error was resolved by means of a Healthy catalog?" confusion -> bug.

Creating an issue to capture the task of "set conditions to false", so that we can follow upstream API conventions for our conditions in future releases, while avoiding confusing bugs.

[anik120]

#3171

[stevekuznetsov]

I think given the context your change is perfectly acceptable. Orthogonally to what Joe's citing, a method that searches for a condition should just return nil or add some boolean to the signature and return false when no such condition exists on the object. Returning some default value and imposing some semantic on top is surprising. For instance, if we were to be writing a controller using this GetCondition() function and we wanted to know - have we already set the thing to Unknown or do we need to add one? Today, not possible with GetCondition().

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: anik120, stevekuznetsov
Once this PR has been reviewed and has the lgtm label, please assign awgreene for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment