Skip to content

bug: set config env separator to double underscore.#763

Merged
tublitzed merged 1 commit intomasterfrom
bug/762
Aug 6, 2020
Merged

bug: set config env separator to double underscore.#763
tublitzed merged 1 commit intomasterfrom
bug/762

Conversation

@jrconlin
Copy link
Member

@jrconlin jrconlin commented Aug 5, 2020

Closes #762

Description

Passing the default separator of "." can cause problems with some shells. It's better to use something like a double underscrore, which is more widely accepted.

Testing

run the application, specifying the limits.max_post_records set to some value, e.g.
SYNC_LIMITS__MAX_POST_RECORDS=99 cargo run -- --config sync.ini

You should now be able to see the "debug_client" value appear in the /info/configuration response:

> curl http://localhost:8000/1.5/1/info/configuration
{"max_post_bytes":2097152,"max_post_records":99,"max_record_payload_bytes":2097152,"max_request_bytes":2101248,"max_total_bytes":100000000,"max_total_records":10000,"debug_client":"1,123"}

Issue(s)

Closes #762.

@jrconlin jrconlin requested a review from a team August 5, 2020 17:14
Copy link
Contributor

@tublitzed tublitzed left a comment

Choose a reason for hiding this comment

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

Can you confirm with updated testing instructions that you've fully tested this locally and that you're seeing the expected over quota error in the sync logs (and associated sentry error) when trying to sync from desktop before we merge this?

@jrconlin
Copy link
Member Author

jrconlin commented Aug 6, 2020

I'm not sure that's required because the actual fix is to allow for environment provided variables to alter settings that are contained in configuration subsets. I chose the limits.max_post_records because the actual usage case for this (limits.debug_client) is a temporary patch enabled for stage testing.

Since the /info/configuration call returns the contents of the limits configuration subset, if the value matches what was provided from the environment variable (and not what was present as either the default, or what was provided in any configuration file) then we can rest assured that the value is being used appropriately in other places in the code.

I will modify the testing steps to change this to match the limits.debug_client since that matches the more "real-world" usage case.

Copy link
Contributor

@tublitzed tublitzed left a comment

Choose a reason for hiding this comment

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

JR and I just chatted about this offline. Approving; let's get this deployed to stg so we can test.

@tublitzed tublitzed merged commit f1d88fe into master Aug 6, 2020
@tublitzed tublitzed deleted the bug/762 branch August 6, 2020 15:53
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.

Sub-section environment configuration options not being read properly

2 participants