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.

Sign up for GitHub

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

Jump to bottom

provider/aws: Add aws elastic beanstalk solution stack #14944

Merged
merged 10 commits into from
May 31, 2017
Merged

provider/aws: Add aws elastic beanstalk solution stack #14944

merged 10 commits into from
May 31, 2017

Conversation

thomaschaaf
Copy link
Contributor

Adds: #14749

Thomas Schaaf added 2 commits May 30, 2017 22:04
Add aws elastic beanstalk solution stack
0acfc9e 
Signed-off-by: Thomas Schaaf <thomaschaaf@Thomass-MBP.fritz.box>
Fix incorrect naming
bc63716 
Signed-off-by: Thomas Schaaf <thomaschaaf@Thomass-MBP.fritz.box>
@thomaschaaf thomaschaaf changed the title [WIP] Add aws elastic beanstalk solution stack [WIP] provider/aws: Add aws elastic beanstalk solution stack May 31, 2017
Use unique go variable/function names
f8e3a42 
Signed-off-by: Thomas Schaaf <thomaschaaf@Thomass-MacBook-Pro.local>
@thomaschaaf thomaschaaf changed the title [WIP] provider/aws: Add aws elastic beanstalk solution stack provider/aws: Add aws elastic beanstalk solution stack May 31, 2017
stack72
stack72 suggested changes May 31, 2017
View reviewed changes
Copy link
Contributor

@stack72 stack72 left a comment

Choose a reason for hiding this comment

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

Hi @thomaschaaf

Thanks for the work here - I have made 1 suggestion around required params and left a question on the sorting

Apart from that, your docs need adding to aws.erb

If we can get these added / changed then this LGTM

Thanks

Paul

builtin/providers/aws/data_source_aws_elastic_beanstalk_solution_stack.go Outdated
func dataSourceAwsElasticBeanstalkSolutionStackRead(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).elasticbeanstalkconn

nameRegex, nameRegexOk := d.GetOk("name_regex")
Copy link
Contributor

Choose a reason for hiding this comment

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

We can do this a different way. If we mark the schema param as required, then we won't need to use d.GetOk and it will validate at plan time

builtin/providers/aws/data_source_aws_elastic_beanstalk_solution_stack.go
}

var solutionStack *string
if len(filteredSolutionStacks) < 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

we can say if len == 0 rather than < 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it as desired

builtin/providers/aws/data_source_aws_elastic_beanstalk_solution_stack.go

// Returns the most recent solution stack out of a slice of stacks.
func mostRecentSolutionStack(solutionStacks []*string) *string {
return solutionStacks[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

is this guaranteed to be the most recent? Don't use need to sort these using a date or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The API returns them sorted. The newest should thus always be first. 🤞

@stack72 stack72 added the waiting-response An issue/pull request is waiting for a response from the community label May 31, 2017
@thomaschaaf
Copy link
Contributor Author

@stack72 hopefully now :)

@stack72
Copy link
Contributor

stack72 commented May 31, 2017

Hi @thomaschaaf

Thanks for fixing these up - unfortunately, the acceptance test doesn't pass

% make testacc TEST=./builtin/providers/aws/ TESTARGS='-run=TestAccAWSElasticBeanstalkSolutionStackDataSource'                               ✭
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/06/01 00:03:59 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws/ -v -run=TestAccAWSElasticBeanstalkSolutionStackDataSource -timeout 120m
=== RUN   TestAccAWSElasticBeanstalkSolutionStackDataSource
--- FAIL: TestAccAWSElasticBeanstalkSolutionStackDataSource (23.14s)
	testing.go:280: Step 0 error: Check failed: Check 2/2 error: data.aws_elastic_beanstalk_solution_stack.multi_docker: Attribute 'name_regex' didn't match "^64bit Amazon Linux (.*) Multi-container Docker (.*)$", got "^64bit Amazon Linux (.*) Multi-container Docker (.*)$"
FAIL
exit status 1
FAIL	github.com/hashicorp/terraform/builtin/providers/aws	23.163s
make: *** [testacc] Error 1

Can you take a look?

Paul

@thomaschaaf
Copy link
Contributor Author

Hey Paul/@stack72 ,

I didn't know about these tests and thought as travis is green everything was working.
Now this test is green too :)

Thomas

@stack72
Copy link
Contributor

stack72 commented May 31, 2017

Awesome! this LGTM now

% make testacc TEST=./builtin/providers/aws/ TESTARGS='-run=TestAccAWSElasticBeanstalkSolutionStackDataSource'                               ✭
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/06/01 02:19:40 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws/ -v -run=TestAccAWSElasticBeanstalkSolutionStackDataSource -timeout 120m
=== RUN   TestAccAWSElasticBeanstalkSolutionStackDataSource
--- PASS: TestAccAWSElasticBeanstalkSolutionStackDataSource (24.85s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	24.884s

@stack72 stack72 merged commit 79c91e1 into hashicorp:master May 31, 2017
@thomaschaaf thomaschaaf deleted the data_source_aws_elastic_beanstalk_solution_stack branch June 1, 2017 06:03
@grubernaut grubernaut mentioned this pull request Jun 1, 2017
Merged
@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.
Reviewers

@stack72 stack72 stack72 requested changes

Assignees
No one assigned
Labels
enhancement provider/aws waiting-response An issue/pull request is waiting for a response from the community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thomaschaaf @stack72 @tombuildsstuff