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

refactor: enhance clarity by changing 'parse_secret_string_as_json' option to 'json #2179

Merged
merged 2 commits into from
Dec 18, 2024

Conversation

isatyamks
Copy link
Contributor

@isatyamks isatyamks commented Dec 14, 2024

Updated the docstring to reflect the correct option name json instead of the incorrect parse_secret_string_as_json

    If the secret contains a string payload ("SecretString"):
    - if the `json` option is True (default):
        {SecretString} will be parsed as a JSON. If successfully parsed, AND the JSON contains a
        top-level object, each entry K/V in the object will also be converted to an entry in the result. V will
        always be casted to a string (if not already a string).
    - If `json` option is False:
        {SecretString} will be returned as a single entry in the result, with the key being the secret_id.n.

{SecretString} will be parsed as a JSON. If successfully parsed, AND the JSON contains a
top-level object, each entry K/V in the object will also be converted to an entry in the result. V will
always be casted to a string (if not already a string).
- If `parse_secret_string_as_json` option is False:
- If `json` option is False:
Copy link
Collaborator

@saikonen saikonen Dec 18, 2024

Choose a reason for hiding this comment

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

as we're updating the docstring to match behaviour, could you also add for the json=False case that the returned secret key is either the secret_id OR the value set by options={"env_var_name": custom_env_var_name}. I leave the phrasing up to you :)

see L149:153 for that code path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @saikonen Now i think the docstring is aligned with the actual behavior and provides a clear explanation of the json=False case.

@isatyamks
Copy link
Contributor Author

@saikonen Since the PR has already been approved and all checks have passed, could you please proceed with merging it?

@savingoyal savingoyal merged commit cd6ed29 into Netflix:master Dec 18, 2024
29 checks passed
@isatyamks isatyamks changed the title refactor: rename parse_secret_string_as_json option to json for clarity refactor: enhance clarity by changing 'parse_secret_string_as_json' option to 'json Dec 18, 2024
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.

3 participants