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

provider/aws: New SSM Parameter resource #15035

Merged
merged 7 commits into from
Jun 6, 2017

Conversation

pmorton
Copy link
Contributor

@pmorton pmorton commented Jun 2, 2017

Hi - I have taken the work from #14043 / #11433 . I have done the following:

  1. Addressed the code review comments
  2. Added the ability to specify the KMS key to be used for SecureString
  3. ForceNew when the type is changed (AWS Imposed Limitation)
  4. Added a data source for ssm parameters

@tomelliff - Did most of the work, but since this is such a great community I thought I would pitch in an bring it over the finish line. Plus I could really use this provider ;)

@radeksimko Since you did the review on the other PR, ping. If you have free time.

tomelliff and others added 5 commits June 2, 2017 10:56
Can be used for creating parameters in AWS' SSM Parameter Store that can then be used by other applications that have access to AWS and necessary IAM permissions.
- Addressed all issues in hashicorp#14043
- Added ForceNew directive to type
- Added the ability to specify a KMS key for encryption and decryption
@ozbillwang
Copy link

ozbillwang commented Jun 3, 2017

@pmorton

Nice job. I hope this PR can be merged ASAP.

Do you mean #14043 can be closed directly?

@pmorton
Copy link
Contributor Author

pmorton commented Jun 3, 2017

@ozbillwang The PR is meant to be a replacement for #14043, so closing 14043 is likely a reasonable choice. Let me know if there is anything else I can do.

@radeksimko
Copy link
Member

Hi @pmorton
thank you for taking the time to address my comments regarding the resource.

Added a data source for ssm parameters

Just a friendly advice for future contributions - we always prefer smaller PRs with ideally just single data source/resource. It's much easier to review as it's less LOC. 😅

At the moment I can't merge this because the data source test is failing - most likely because it's expecting test.parameter to exist. We expect all acceptance tests to be as self sustainable as possible - that means we expect the test to also create such parameter before testing the behaviour of the data source.

See https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/data_source_aws_alb_test.go#L51 for example of how we test ALB data source.

=== RUN   TestAccAwsSsmParameterDataSource_basic
--- FAIL: TestAccAwsSsmParameterDataSource_basic (6.57s)
	testing.go:280: Step 0 error: Error refreshing: 1 error(s) occurred:

		* data.aws_ssm_parameter.test: 1 error(s) occurred:

		* data.aws_ssm_parameter.test: data.aws_ssm_parameter.test: [ERROR] SSM Parameter test.parameter is invalid
FAIL

@radeksimko radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Jun 4, 2017
@pmorton
Copy link
Contributor Author

pmorton commented Jun 5, 2017

@radeksimko Thanks for the guidance. I have since reviewed the contribution guide (which was super helpful). I have fixed the integration tests. Let me know the next steps. Cheers.

@radeksimko radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Jun 5, 2017
@radeksimko radeksimko self-assigned this Jun 5, 2017
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

LGTM, I just fixed one small typo in the docs - I hope you don't mind.

@radeksimko radeksimko merged commit e4899de into hashicorp:master Jun 6, 2017
@tomelliff
Copy link
Contributor

@pmorton thanks for finishing this off, I've been utterly swamped since starting a new job so haven't had any time to look at this. Looking forward to being able to use this in the next release!

@ozbillwang
Copy link

So nice. It has been released in v0.9.7

@ghost
Copy link

ghost commented Apr 11, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants