OCPVE-717: fix: add if check for no resource match error#801
Conversation
ffromani
left a comment
ffromani
left a comment
There was a problem hiding this comment.
LGTM, but how can we test this change?
|
/hold Had an error pop up in validation, I had validated this working with a small app with a release that had no OLM, however rebuilding the build with this fix seems to not resolve the error, not sure if cluster bot picked up the right SHA. Will validate tomorrow morning. @ffromani Unfortunately, There is no great way to run a pipeline of this working since we're in the early development of removing OLM and It requires changes in a few operators for the feature to work properly. I will run another cluster with this change in tomorrow and make sure it works as intended. |
openshift-ci
Bot
added
the
do-not-merge/hold
verification is surely welcome, but in general this flow is relatively complex and fragile (because it runs rarely and only in special cases). It would be great to have better test coverage to increase the confidence. |
eggfoobar
force-pushed
the
fix-optional-olm-install
branch
from
7b590aa to
63f73d7
Compare
|
More tests are always better, I'm not entirely sure of the best way to test this part, but since I'm already poking around there I can try and take a stab at an e2e test if that's possible for an upgrade cycle. Unit test might be best here, what do you think? Side note, the extra if checks won't change the behavior of that method so we're good there, this just adds an extra check for when the OLM goes optional. |
eggfoobar
force-pushed
the
fix-optional-olm-install
branch
from
63f73d7 to
8cad7fa
Compare
eggfoobar
changed the title
|
@eggfoobar: This pull request references OCPVE-717 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set. DetailsIn 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. |
openshift-ci-robot
added
the
jira/valid-reference
eggfoobar
force-pushed
the
fix-optional-olm-install
branch
from
8cad7fa to
764786b
Compare
|
/unhold After some research, the issue in validation was around the error being wrapped and discovery helper func not being able to handle that scenario. I added a util helper to check. @ffromani Seems the unit test might be the best approach here, what do you think? |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
upkeep, removed redundant if nill check Signed-off-by: ehila <ehila@redhat.com> fix: added util to help parse error from discovery and meta error Signed-off-by: ehila <ehila@redhat.com> fix: added unit tests for error check Signed-off-by: ehila <ehila@redhat.com>
eggfoobar
force-pushed
the
fix-optional-olm-install
branch
from
764786b to
7408f0b
Compare
|
@eggfoobar: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
@ffromani When you get a chance, this should be good now for a final review. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eggfoobar, yanirq The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@eggfoobar: This pull request references OCPVE-717 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set. DetailsIn 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. |
|
@eggfoobar: This pull request references OCPVE-717 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set. DetailsIn 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. |
5 participants
OLM will be made optional in 4.15, when OLM resources are removed, the error check fails to be caught with
IsNotFoundresulting in a false positive error. Updating to also check forNoResourceMatchError