Skip to content

Conversation

@demolitionmode
Copy link
Contributor

What does this PR do?

  • Adds option to fetch API Key from AWS SSM.
  • Updates Parameters description to show all the options

Motivation

I use the parameter store for all other things, I like to be consistent where possible.

Additional Notes

Related issue: #195

@DarcyRaynerDD
Copy link
Contributor

Hi marshall0705 👋
Thanks for the PR, this looks super useful. I left one small nitpick, but otherwise we should be good to add this to the next release.

@demolitionmode
Copy link
Contributor Author

Hi @DarcyRaynerDD
That's good to hear. I welcome all comments, even nitpicking 😄, however I don't seem to be able to see your comment.

DD_API_KEY = boto3.client("secretsmanager").get_secret_value(
SecretId=SECRET_ARN
)["SecretString"]
elif "DD_API_KEY_SSM_NAME" in os.environ:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we rename this it DD_SSM_API_KEY, just to keep it consistent with DD_KMS_API_KEY

Copy link
Contributor

@DarcyRaynerDD DarcyRaynerDD Jan 28, 2020

Choose a reason for hiding this comment

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

@marshall0705, I forgot to submit the review, doh

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 had considered naming it that. It was a choice between the convention used for the secrets manager var, and the convention used for the KMS var.
I chose this name due to DD_KMS_API_KEY and DD_API_KEY both actually containing the key (albeit one of them encrypted), whereas DD_API_KEY_SECRET_ARN is a reference to a remote location, as is DD_API_KEY_SSM_NAME.
If you would still like the name changing, I'm fine to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, I think we can keep your naming convention then. I'll merge this and roll it out into the next release.

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