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

adds ssm backend #71

Merged
merged 8 commits into from
Sep 26, 2019
Merged

adds ssm backend #71

merged 8 commits into from
Sep 26, 2019

Conversation

steamrolla
Copy link
Contributor

Adds Amazon SSM's Parameters as a backend.

There are many ways the calls could be made to Amazon, but, just starting with the simplest now. User just gives the path for their parameter store. Can be easily changed in the future if people want to use it in a different way. Very open to ideas here.

Copy link
Contributor

@rogpeppe rogpeppe left a comment

Choose a reason for hiding this comment

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

LGTM modulo a bunch of minor comments and suggestions. Thanks very much indeed for doing this!

ptr/ptr.go Outdated Show resolved Hide resolved
backend/ssm/ssm.go Outdated Show resolved Hide resolved
backend/ssm/ssm.go Outdated Show resolved Hide resolved
backend/ssm/ssm.go Outdated Show resolved Hide resolved
mocks/SSMAPI.go Outdated Show resolved Hide resolved
backend/ssm/ssm_test.go Outdated Show resolved Hide resolved
Gopkg.toml Outdated Show resolved Hide resolved
backend/ssm/ssm_test.go Show resolved Hide resolved
backend/ssm/ssm.go Outdated Show resolved Hide resolved
@steamrolla
Copy link
Contributor Author

Thanks for the review! Will get these changes in soon.

@steamrolla
Copy link
Contributor Author

@rogpeppe, I think I got them all! Let me know if anything still needs changed.

Copy link
Contributor

@rogpeppe rogpeppe left a comment

Choose a reason for hiding this comment

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

I have a few minor issues with this, but you've given enough of your time, thanks! I'll merge this and make some changes later...


before_script:
- docker run -d -p 2379:2379 quay.io/coreos/etcd /usr/local/bin/etcd -advertise-client-urls http://0.0.0.0:2379 -listen-client-urls http://0.0.0.0:2379
- docker run -d -p 8500:8500 --name consul consul
- docker run -d -p 8200:8200 --cap-add=IPC_LOCK -e 'VAULT_DEV_ROOT_TOKEN_ID=root' -e 'VAULT_DEV_LISTEN_ADDRESS=0.0.0.0:8200' vault:0.9.6
- docker run -d -p 8200:8200 --cap-add=IPC_LOCK -e 'VAULT_DEV_ROOT_TOKEN_ID=root' -e 'VAULT_DEV_LISTEN_ADDRESS=0.0.0.0:8200' vault:0.9.6
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add the final newline here?

cache map[string][]byte
}

func NewBackend(ssm ssmiface.SSMAPI, path string) *Backend {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a doc comment explaining what this does and how this backend interacts with SSM (presumably path specifies a path within the SSM parameters, but it would be great to be explicit about this).

Also, I think it would be better to be consistent with the other backends and return confita.Backend here (and unexport the Backend struct type), as I don't think we want to add extra methods in the future, and this is all about returning a confita backend instance.

ssmiface.SSMAPI
}

func (_m *mockSSM) GetParametersByPathWithContext(_a0 context.Context, _a1 *ssm.GetParametersByPathInput, _a2 ...request.Option) (*ssm.GetParametersByPathOutput, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why all these variables are named with leading underscores.
This looks like generated code, but I don't see that there's really a need for generated code here.

@rogpeppe rogpeppe merged commit 1833a32 into heetch:master Sep 26, 2019
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