-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Elastic Beanstalk support for PlatformArn #6093
Elastic Beanstalk support for PlatformArn #6093
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jbienkowski311 👋 Thanks for submitting this! A few minor little things then this should be good to go -- please let us know if you have any questions or do not have time to implement the feedback.
Config: testAccBeanstalkEnvConfig_platform_arn(appName, envName, platformArn), | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckBeanstalkEnvExists("aws_elastic_beanstalk_environment.tfenvtest", &app), | ||
resource.TestMatchResourceAttr( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified to:
resource.TestCheckResourceAttr("aws_elastic_beanstalk_environment.tfenvtest", "platform_arn", platformArn),
name = "%s" | ||
application = "${aws_elastic_beanstalk_application.tftest.name}" | ||
platform_arn = "%s" | ||
depends_on = ["aws_elastic_beanstalk_application.tftest"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two nits:
- The formatting seems a bit off here (maybe spaces vs tabs?)
- The
depends_on
is unnecessary as its already explicitly declared byaws_elastic_beanstalk_application.tftest.name
Hi @bflad 👋 Thanks for the review. I think I have fixed all the pointed out issues. The test still passes 😉
|
Yay! Thats great news, although it looks like AWS will automatically fill in
My recommendation would be to add |
Hi @bflad, I have rerun the whole
|
Regarding hitting the acceptance testing timeout, once Terraform 0.11.9 is tagged and released, I will pull in the dependency and implement #5874 In the meantime, you can temporarily (we don't want to mess up the above process) add |
Sounds good to me! What about the failing test? Should I fix it as well, or is it out of the scope for this PR? Maybe you can confirm that it is failing for you as well (maybe my AWS account/credentials are misconfigured)? |
Interestingly enough that test passes in our CI environment with |
I used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @jbienkowski311! 🚀
This has been released in version 1.40.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
If you are using AWS's ready to use solutions stacks, then you are fine. But if for some reason you would like to use AWS Custom Platform, then Terraform will not support it.
This PR adds support for CloudFormation's parameter
PlatformArn
, therefore adding support for Custom Platforms to Terraform.Changes proposed in this pull request:
platform_arn
parameter to Elastic Beanstalk Environment resource. More about PlatformArn can be found in AWS DocsOutput from acceptance testing: