-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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: Elastic Beanstalk Application and Environment #3157
Conversation
"aws_ecs_service": resourceAwsEcsService(), | ||
"aws_ecs_task_definition": resourceAwsEcsTaskDefinition(), | ||
"aws_eip": resourceAwsEip(), | ||
"aws_elastic_beanstalk_application": resourceAwsElasticBeanstalkApplication(), |
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.
In #1892 and #2783 (Opsworks and CodeDeploy respectively) we'd standardized on using aws_..._app
. Neither is merged yet of course, so we could change those to use _application
to match with this, but it seems worth deciding what the standard is here so that all of the different overlapping AWS app deployment solutions have similar structure in Terraform.
(I actually closed #1892 because it was becoming a bit of a beast and I wanted to focus on the smaller change in #2162, but I'd like to circle back and finish this one day if my first set of changes ever actually makes it in.)
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.
longer words typically are better to reduce ambiguity, in this case I don't think _app is more ambiguous than _application so it seems a little bit neater to use the shorter form
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.
Indeed... it seems that brevity has been the focus in existing names anyway, with e.g. eip
instead of elastic_ip
... so app
here would be hardly the worst example of abbreviating things.
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.
I think it also important what is the convention amongst users of AWS and in the API.
EIP & ELB are AFAIK commonly known abbreviations used in APIs and elsewhere, whereas ASG isn't, hence we use autoscaling_group
instead of ASG.
Ain't saying that CloudFormation always represents conventions, but FYI:
AWS::OpsWorks::App
AWS::ElasticBeanstalk::Application
AWS::ElasticBeanstalk::ApplicationVersion
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.
Perhaps I'm foolish for trying to hold Terraform UX to a higher standard of consistency than the APIs that underpin it. 😀
23903cb
to
4e1993b
Compare
Bare minimum tag implementation for Elastic Beanstalk Application and Environment.
4e1993b
to
f81984c
Compare
@phinze I know you're out and about, but when you get a chance I'd love a review here, thanks! |
Computed: true, | ||
Type: schema.TypeString, | ||
Optional: true, | ||
Computed: true, |
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.
extra whitespace diff here
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.
What is this even doing here?
I assume it's from a rebase or something.
Looking good generally! Few comments inline. |
I tried to test out this feature with the following Terraform config:
But I get the following error:
Thanks for doing this work - it'll be awesome to have EB support in Terraform! |
Provides an Elastic Beanstalk Environment Resource | ||
--- | ||
|
||
# aws\_elastic\_beanstalk\_<wbr>Environment |
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.
I think the "e" in "Environment" here was intended to be lowercase
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.
Fixed in cbc010d
Nice work! Can't wait for this one! |
I've been testing this out with a single EB app. A couple features that would be nice to have:
Also, I noticed that after creating an EB app with a few settings defined (such as a VPCId and subnets) the list of settings is always re-ordered when I run |
Are there any plans to merge this feature into master? |
@gpetras if you have a sample configuration that demonstrates this, I'd be happy to take another look. We definitely shouldn't have reoccurring plans |
@vladnik we'd love to merge this as soon as possible, but I was hoping for some more community vetting to occur before doing so. |
* master: (191 commits) Update CHANGELOG.md Update CHANGELOG.md Use hc-releases provider/google: Added scheduling block to compute_instance Use vendored fastly logo Use releases for releases Update CHANGELOG.md Update CHANGELOG.md Update vpn.tf Update CHANGELOG.md Update list of backends in RemoteConfigCommand's Help() method Update list of GCE service scope short names provider/google: Fixed timeout bug on large instance groups release: clean up after v0.6.6 v0.6.6 Update CHANGELOG.md Fixing the cloudwatch_metric_alarm auto scale example Add `computed` flag to the `network_domain` parameter Update CHANGELOG.md Nicer error when list/map assigned to string argument. ...
Here are my thoughts on the current state of this pr. There are a few areas that could use some more attention, but I don't think all of these need to be done before adding the new resources into master.
I think the following points could be addressed after the initial resources are added to Terraform master.
|
I checked out this branch yesterday, and tested a simple workflow that we want to use. This branch seemed to work perfectly. Great work. |
👍 we're using it too, working well, would love to see this on master now. |
Really great. If Atlas allowed me to choose a branch to run terraform with it would be this one. Any chance of seeing this in the next release? |
+1 I'd love to see this in the next release as well! |
Rebased against master on #5349 to help move this along. Feel free to use if it helps or discard. |
Hiya folks - this is on our radar and we'll make sure it gets in before the next release! 👍 |
* master: (394 commits) fix indentation Added disk_size_gb documentation to resource "google_compute_instance_template" website: Fix missing sidebar_current Fix typo (APIGateway -> API Gateway) provider/aws: Sort API Gateway resources alphabetically provider/aws: Remove unnecessary GetChange in Delete funcs aws/docs: Fix wrong field name (parent_resource_id -> parent_id) provider/aws: Guard APIGateway resource & REST API against deletion vendor: Update github.com/aws/aws-sdk-go/service/apigateway Update CHANGELOG.md Add aws_api_gateway_deployment resource Add aws_api_gateway_api_key resource Add aws_api_gateway_model resource Add aws_api_gateawy_integration_response resource Add aws_api_gateway_integration resource Add aws_api_gateway_method_response resource Add aws_api_gateway_method resource Add aws_api_gateway_resource resource Add aws_api_gateway_rest_api resource Vendor AWS APIGateway API ...
Sort subnets to avoid spurious diffs
…orm into f-aws-elastic-beanstalk * 'f-aws-elastic-beanstalk' of github.com:hashicorp/terraform: Sort subnets to avoid spurious diffs
provider/aws: Elastic Beanstalk Tier Attribute
…orm into f-aws-elastic-beanstalk * 'f-aws-elastic-beanstalk' of github.com:hashicorp/terraform: Adding tier attribute and associated test to aws_elastic_beanstalk_environment.
Hey all – First, a huge thank you for everyone who contributed here, in terms of code, ideas, feedback, and overall just time with this PR. Second, a huge sorry for not getting this in sooner :/ Third, I'm going to merge this now. Thanks again 😄 |
* master: insert missing word Update CHANGELOG.md Return empty string when input empty S3 bucket policy Fix docker test assertions regarding latest tag
I kind of lied – I didn't merge this branch as is, I squashed it into f0d3176 |
Hi. Is "aws_elastic_beanstalk_application_version" usable? It's not recognized by my Terraform 0.6.14. Thank you. resource "aws_elastic_beanstalk_application_version" "default" { Errors:
|
My apologise if this is known already. I run into an issue with something like the following:
Getting a "InvalidParameterValue: The scheduled action name cannot be blank." error. Apparently the namespace "aws:autoscaling:scheduledaction" expects a scheduled_action_name parameter (http://docs.aws.amazon.com/elasticbeanstalk/latest/dg/command-options-general.html#command-options-general-autoscalingscheduledaction) while "setting" only allows "namespace", "name" and "value" as parameters |
@jefferai and I discussed this on Friday. With three fully-documented SSH backends, the page is lengthy, ungreppable, and intimidating. This commit separates the SSH backends into their own pages with as little text changes as possible.
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. |
This is a work in progress forElastic Beanstalk application/environment resourcesRemaining: