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.

Sign up for GitHub

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

Jump to bottom

fix: make sure file close returns any errors #251

Merged
merged 1 commit into from
May 5, 2023
Merged

fix: make sure file close returns any errors #251

merged 1 commit into from
May 5, 2023

Conversation

joekr
Copy link
Member

@joekr joekr commented May 2, 2023

What this PR does / why we need it:
Using the simple defer is a nice way to close the file, but if the file write/close returns an error we won't know that. Using the deferCloseFunc anonymous function keeps the close near the open call, but still allows returning the close error.

More info here https://www.joeshaw.org/dont-defer-close-on-writable-files/

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

@joekr joekr added the enhancement New feature or request label May 2, 2023
@joekr joekr self-assigned this May 2, 2023
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label May 2, 2023
@joekr
Copy link
Member Author

joekr commented May 2, 2023

unit tests

?       github.com/oracle/cluster-api-provider-oci      [no test files]
ok      github.com/oracle/cluster-api-provider-oci/api/v1beta1  18.976s coverage: 24.0% of statements
ok      github.com/oracle/cluster-api-provider-oci/api/v1beta2  0.712s  coverage: 15.8% of statements
?       github.com/oracle/cluster-api-provider-oci/cloud/config [no test files]
ok      github.com/oracle/cluster-api-provider-oci/cloud/ociutil        0.217s  coverage: 19.6% of statements
ok      github.com/oracle/cluster-api-provider-oci/cloud/scope  231.352s        coverage: 77.2% of statements
?       github.com/oracle/cluster-api-provider-oci/cloud/scope/mocks    [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/base  [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/base/mock_base        [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/compute       [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/compute/mock_compute  [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/computemanagement     [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/computemanagement/mock_computemanagement      [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/containerengine       [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/containerengine/mock_containerengine  [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/identity      [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/identity/mock_identity        [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/loadbalancer  [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/loadbalancer/mock_lb  [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/networkloadbalancer   [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/networkloadbalancer/mock_nlb  [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/vcn   [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/vcn/mock_vcn  [no test files]
ok      github.com/oracle/cluster-api-provider-oci/cloud/util   0.359s  coverage: 55.8% of statements
ok      github.com/oracle/cluster-api-provider-oci/controllers  26.243s coverage: 67.5% of statements
ok      github.com/oracle/cluster-api-provider-oci/exp/api/v1beta1      14.089s coverage: 21.8% of statements
ok      github.com/oracle/cluster-api-provider-oci/exp/api/v1beta2      0.297s  coverage: 10.9% of statements
ok      github.com/oracle/cluster-api-provider-oci/exp/controllers      1.099s  coverage: 56.3% of statements
?       github.com/oracle/cluster-api-provider-oci/feature      [no test files]
?       github.com/oracle/cluster-api-provider-oci/version      [no test files]

e2e tests

Ran 6 of 25 Specs in 3742.683 seconds
SUCCESS! -- 6 Passed | 0 Failed | 0 Pending | 19 Skipped

@joekr joekr marked this pull request as ready for review May 2, 2023 19:10
shyamradhakrishnan
shyamradhakrishnan reviewed May 4, 2023
View reviewed changes
cloud/config/config.go Outdated Show resolved Hide resolved
cloud/config/config.go Outdated Show resolved Hide resolved
@joekr
fix: make sure file close returns any errors
c54c7c4 
Using the simple defer is a nice way to close the file, but if the file
write/close returns an error we won't know that. Using the `deferCloseFunc`
anonymous function keeps the close near the open call, but
still allows returning the close error.
@joekr joekr merged commit cd726b2 into oracle:main May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shyamradhakrishnan shyamradhakrishnan shyamradhakrishnan approved these changes

Assignees

@joekr joekr

Labels
enhancement New feature or request OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@joekr @shyamradhakrishnan