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

Adding support for specifying secrets using a list of strings. #32

Merged
merged 4 commits into from
Sep 16, 2019

Conversation

mccredie
Copy link
Contributor

@mccredie mccredie commented Sep 6, 2019

Issue

#1

What

You can now specify only the ssm name for secrets as a list in the serverless.yml file

provider:
  environmentSecrets:
    - MY_SECRET

is equivalent to

provider:
  environmentSecrets:
    MY_SECRET: MY_SECRET

Test

See the example above. You should be able to create a secret in parameter store with a simple name like 'MY_SECRET', reference it in serverless.yml using the name 'MY_SECRET' in a list, and then reference it in your lambda using the name 'MY_SECRET'.

@mccredie mccredie requested a review from piohhmy September 6, 2019 23:36
Copy link
Contributor

@piohhmy piohhmy left a comment

Choose a reason for hiding this comment

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

Looks great! Let's just iterate on the README.md a bit. I'm happy to take a stab too if you agree with the comment there

README.md Outdated Show resolved Hide resolved
README.md Outdated
environmentSecrets:
MY_SECRET: MY_SECRET
```

The plugin will create a json file with all the secrets. In the above example the ciphertext and ARN of the secret located at `path/to/ssm/secret` will be stored in the file under the key `MYSECRET`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This paragraph references path/to/ssm/secret which doesn't match up with the last example.

What do you all think of having a basic getting started that just uses the 'simple' way, and then create a new Advanced Configuration section that details how to customize the key name. This we we lower the barrier to adoption w/ as little upfront customization as possible, but then provide opportunities for 'advanced' config for power users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good thought.

Co-Authored-By: Danny Varner <[email protected]>
@mccredie
Copy link
Contributor Author

I think we should actually run a functional test too.

@mccredie
Copy link
Contributor Author

Okay, I did some testing on the solution. We also updated the examples to use the list and explicit object syntax. Please review again @piohhmy

Copy link
Contributor

@piohhmy piohhmy left a comment

Choose a reason for hiding this comment

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

That looks great! The ability to mix and match is 👌

@cruisercohen
Copy link
Contributor

@piohhmy Did you run manual tests on the different ways to specify secrets? If not one of us probably should just so there's a second test beyond the original developer environment.

@mccredie mccredie merged commit 18e8a40 into master Sep 16, 2019
@mccredie mccredie deleted the secret-string-list branch September 16, 2019 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants