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

Make sure to return nil value when there was an error #1079

Conversation

kadel
Copy link
Member

@kadel kadel commented Dec 7, 2021

Motivation

Saving service crd (s.crd) when there was an error while retrieving it (s.CrdReader(s.groupVersionResource)) can lead to nil pointer problems.

This was causing problem for odo when it tried to use SBO as a library.

Changes

Return error and don't save crd to s.crd.

Testing

Added a test that covers this scenario.

For further more details refer the CONTRIBUTING.md

Release Notes

Before this update, when the Service Binding Operator was used as a library, there was an error while retrieving a service custom resource definition (CRD). This situation could lead to nil pointer problems and crash the Operator. With this update, the issue is resolved and the Operator is made more stable.

@openshift-ci openshift-ci bot requested review from baijum and isutton December 7, 2021 09:57
@kadel kadel force-pushed the fix-CustomResourceDefinition branch from 4db3bd8 to c49634e Compare December 7, 2021 09:58
@codecov
Copy link

codecov bot commented Dec 7, 2021

Codecov Report

Merging #1079 (7228c97) into master (9ee6062) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 7228c97 differs from pull request most recent head fe89a3e. Consider uploading reports for the commit fe89a3e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1079      +/-   ##
==========================================
+ Coverage   58.87%   58.92%   +0.04%     
==========================================
  Files          30       30              
  Lines        1668     1670       +2     
==========================================
+ Hits          982      984       +2     
  Misses        560      560              
  Partials      126      126              
Impacted Files Coverage Δ
pkg/reconcile/pipeline/context/service/service.go 71.05% <100.00%> (+0.51%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ee6062...fe89a3e. Read the comment docs.

Copy link
Contributor

@pedjak pedjak left a comment

Choose a reason for hiding this comment

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

looks good - we need just to tighten error assertion in the new test.

Expect(err).NotTo(HaveOccurred())

res, err := service.CustomResourceDefinition()
Expect(err).To(HaveOccurred())
Copy link
Contributor

@pedjak pedjak Dec 7, 2021

Choose a reason for hiding this comment

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

the error should occur and should be the very same that gets returned in line 100.

Copy link
Contributor

@pedjak pedjak left a comment

Choose a reason for hiding this comment

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

/lgtm please squash commits

When CrdReader fail and returns error other than NotFound
service.CustomResourceDefinition should return 'nil' instead of CRD that
might be invalid.

Signed-off-by: Tomas Kral <tkral@redhat.com>
@kadel kadel force-pushed the fix-CustomResourceDefinition branch from 7228c97 to fe89a3e Compare December 8, 2021 10:12
@kadel
Copy link
Member Author

kadel commented Dec 8, 2021

/lgtm please squash commits

done. thx for review.

Copy link
Contributor

@pedjak pedjak left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 8, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pedjak

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Dec 8, 2021
@openshift-merge-robot openshift-merge-robot merged commit 1f72711 into redhat-developer:master Dec 8, 2021
@jasperchui jasperchui added this to the post-1.0 milestone Feb 16, 2022
@pmacik pmacik added the kind/bug Something isn't working label Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved kind/bug Something isn't working lgtm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants