Skip to content

Conversation

codeodor
Copy link
Contributor

@codeodor codeodor commented Mar 20, 2024

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

I did not validate it against all platform versions, but rather than use something like present? or try or &. I just used the ruby idiom for checking nil/false by using the object at the end, like && object

Have you considered putting that last requirement into the CI's hands so that it might become unnecessary?

Related issues

None that I am aware of.

Describe the solution you've provided

The code previously checked that Rails responded to logger and if it did, assumes there is actually a logger present, which is not always the case. This leaves LD with a nil value for logger which causes the application to crash.

Describe alternatives you've considered

I did not consider any alternatives.

Additional context

None.

Rails logger could be nil, leaving us with a nil value for logger, and unable to load the application due to the assumed presence of a logger.
@codeodor codeodor requested a review from a team March 20, 2024 21:52
@codeodor codeodor changed the title Check that Rails.logger exists before using it Fix: Do not assume Rails.logger exists simply because it responds to the method Mar 20, 2024
@codeodor codeodor changed the title Fix: Do not assume Rails.logger exists simply because it responds to the method fix: Do not assume Rails.logger exists simply because it responds to the method Mar 20, 2024
@keelerm84 keelerm84 merged commit 2f0f7ed into launchdarkly:main Mar 21, 2024
@keelerm84
Copy link
Member

Thank you for this contribution!

@codeodor
Copy link
Contributor Author

My pleasure!

keelerm84 pushed a commit that referenced this pull request Mar 28, 2024
🤖 I have created a release *beep* *boop*
---


##
[8.3.1](8.3.0...8.3.1)
(2024-03-28)


### Bug Fixes

* Ensure Rails.logger exists before using it
([#258](#258))
([2f0f7ed](2f0f7ed))
* Remove invalid prereq `check_variation_range` check
([#261](#261))
([#263](#263))
([74ca206](74ca206)),
closes
[#260](#260)

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

2 participants