Skip to content

Conversation

@mwildehahn
Copy link
Contributor

No description provided.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about making this more generic so that it's used by every translator?

Also is it worth using urlparse instead of the raw split you do here? You could parse it with urlparse, and if there is no scheme, then just use the raw value? Maybe it's not worth it at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i refactored this to a function that could potentially be used by other translators. i'd wait on making it more complex until we need to

@mwildehahn
Copy link
Contributor Author

not sure if we should cache the result of _get_config_directory since we have argparse open the file and parse the env file, it could have some overhead if you have multiple kms values

Copy link
Member

Choose a reason for hiding this comment

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

Both of these helper functions seem useful/generic enough that I think they should probably be in the base translators module - I could see other translators, in the future, wanting to use them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense

@phobologic
Copy link
Member

While we don't have full docs, this feature is useful enough that I think it might be worth mentioning the use of file:// as well as just putting a string in the field in the section of the README.rst that discusses translators.

@phobologic
Copy link
Member

👍

phobologic added a commit that referenced this pull request Jan 7, 2016
Support storing the kms encrypted value in a file
@phobologic phobologic merged commit 0bd4183 into cloudtools:master Jan 7, 2016
@mwildehahn mwildehahn deleted the file-prefix-kms branch January 8, 2016 18:52
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