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

Refinery overwrites config settings by applying defaults after config was loaded #954

Closed
fchikwekwe opened this issue Dec 18, 2023 · 4 comments
Closed

Refinery overwrites config settings by applying defaults after config was loaded #954

fchikwekwe opened this issue Dec 18, 2023 · 4 comments
Assignees
@fchikwekwe
Labels
type: bug Something isn't working
Milestone
v2.4

Comments

@fchikwekwe
Copy link
Contributor

As seen in this section of code, Refinery is loading in defaults after the config has already been loaded in. This can result in user settings being reset to default. We need to apply defaults first and then load in the config to overwrite sections with user settings.
@fchikwekwe fchikwekwe added type: bug Something isn't working status: oncall Flagged for awareness from Honeycomb Telemetry Oncall labels Dec 18, 2023
@fchikwekwe fchikwekwe removed the status: oncall Flagged for awareness from Honeycomb Telemetry Oncall label Dec 19, 2023
@fchikwekwe fchikwekwe added this to the v2.4 milestone Dec 21, 2023
@fchikwekwe fchikwekwe self-assigned this Jan 2, 2024
@robbkidd
Copy link
Member

robbkidd commented Jan 17, 2024 •
edited
Loading

After our coding session today, I poked around at creasty/defaults and found their test cases for "don't override an existing value with a default." I think this change demonstrates our failure scenario: a true default appearing as the final value despite false being set by the user.

@fchikwekwe
Copy link
Contributor Author

Opened an issue while still working on this creasty/defaults#49.

@robbkidd robbkidd mentioned this issue Jan 18, 2024
Merged
4 tasks
@fchikwekwe fchikwekwe mentioned this issue Jan 19, 2024
Closed
3 tasks
@fchikwekwe
Copy link
Contributor Author

Closed by #969. Upstream work creasty/defaults#50 and creasty/defaults#51.

@fchikwekwe
Copy link
Contributor Author

Followup worked to be done for 2.5 release described in #970.

fchikwekwe added a commit that referenced this issue Jan 25, 2024
@robbkidd @fchikwekwe
fix: allow config bools to default to true (#969)
451d00e 
## Which problem is this PR solving?

- #954 

## Short description of the changes

- add a failing test to reproduce the problem
- updated config struct fields that are bools defaulting to true to be
bool pointers so that nil represents the unset state
- updated getter functions to handle the pointers
- [x] add a GetSamplerEnabled() function to HoneycombLoggerConfig to
handle pointer defreferencing

## Maybe TODO

- [ ] use something (reflect?) to do struct tag lookups to find default
values in the getter functions instead of declaring defaults in multiple
places
- [ ] keep the default-true-bool-overridden-with-false test?
- [ ] contribute documentation about bool pointer workaround to upstream
defaults library?

---------

Co-authored-by: Faith Chikwekwe <[email protected]>
Co-authored-by: Faith Chikwekwe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fchikwekwe fchikwekwe

Labels
type: bug Something isn't working
Projects
None yet
Milestone
v2.4
Development

No branches or pull requests
3 participants
@robbkidd @MikeGoldsmith @fchikwekwe