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: Elastic Beanstalk Application and Environment #3157

Closed
wants to merge 42 commits into from

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Sep 2, 2015

This is a work in progress for Elastic Beanstalk application/environment resources

Remaining:

  • Tests
  • Documentation

"aws_ecs_service": resourceAwsEcsService(),
"aws_ecs_task_definition": resourceAwsEcsTaskDefinition(),
"aws_eip": resourceAwsEip(),
"aws_elastic_beanstalk_application": resourceAwsElasticBeanstalkApplication(),
Copy link
Contributor

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.)

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

Copy link
Contributor

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.

Copy link
Member

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

Copy link
Contributor

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. 😀

@radeksimko radeksimko added the wip label Sep 3, 2015
@catsby catsby force-pushed the f-aws-elastic-beanstalk branch 2 times, most recently from 23903cb to 4e1993b Compare October 8, 2015 15:15
Bare minimum tag implementation for Elastic Beanstalk Application and
Environment.
@catsby catsby force-pushed the f-aws-elastic-beanstalk branch from 4e1993b to f81984c Compare October 8, 2015 15:16
@catsby
Copy link
Contributor Author

catsby commented Oct 8, 2015

@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,
Copy link
Contributor

Choose a reason for hiding this comment

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

extra whitespace diff here

Copy link
Contributor Author

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.

@phinze
Copy link
Contributor

phinze commented Oct 8, 2015

Looking good generally! Few comments inline.

@radeksimko radeksimko removed the wip label Oct 8, 2015
@apparentlymart apparentlymart changed the title provider/aws: Elastic Beanstalk WIP provider/aws: Elastic Beanstalk Application and Environment Oct 10, 2015
@gpetras
Copy link

gpetras commented Oct 22, 2015

I tried to test out this feature with the following Terraform config:

resource "aws_elastic_beanstalk_application" "testapp" {
  name = "testapp"
  description = "Test App"
}

resource "aws_elastic_beanstalk_environment" "development" {
  name = "development-testapp"
  application = "${aws_elastic_beanstalk_application.testapp.name}"
  cname = "myappname"
  solution_stack_name = "64bit Amazon Linux 2015.03 v1.4.3 running Docker 1.6.2"
}

But I get the following error:

Errors:

  * aws_elastic_beanstalk_environment.testapp: "cname": this field cannot be set

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
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in cbc010d

@tobycox
Copy link

tobycox commented Oct 26, 2015

Nice work! Can't wait for this one!
I haven't contributed before, but if there's anything left to do that I could try to help with — let me know.

@gpetras
Copy link

gpetras commented Oct 26, 2015

I've been testing this out with a single EB app. A couple features that would be nice to have:

  • access to additional exported resources, specifically ELB DNS fqdn and hosted zone id
    • so that route 53 DNS ALIAS records can be associated to the EB application's ELB
  • ability to specify DNS cname instead of having one auto-generated

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 terraform plan or terraform apply. Not sure if there's a way to sort the list so that it doesn't come up as a change in the execution plan each time. I can provide more details on my configuration if that is needed.

@vladnik
Copy link

vladnik commented Oct 27, 2015

Are there any plans to merge this feature into master?

@catsby
Copy link
Contributor Author

catsby commented Oct 27, 2015

@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

@catsby catsby added the waiting-response An issue/pull request is waiting for a response from the community label Oct 27, 2015
@catsby
Copy link
Contributor Author

catsby commented Oct 27, 2015

@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.
  ...
@dharrisio
Copy link
Contributor

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.

  • provider/aws: Elastic Beanstalk Application Version #3871 pr merged would be good, since we rely on this functionality for our Terraform/Beanstalk usage. There is currently one issue that I am seeing
    when using the depends_on option. I'm not sure on this one, but I think it might be releated to an issue in Terraform core. Other than that, I think
    it just needs to be reviewed.
  • provider/aws: Elastic Beanstalk Tier Attribute #5209 necessary for 'Worker' tier environments.
  • Configuration Templates: We aren't using these resources, so I can't say for sure if more work needs to be done there.
    Perhaps we just need to add a test for the elastic_beanstalk_environment resource verifying that elastic_beanstalk_configuration_template
    gets attached to the environment correctly.

I think the following points could be addressed after the initial resources are added to Terraform master.

  • Recurring plans: This is a bit of an annoying issue, because there hasn't been a good general solution that fits all of the different option_settings
    that can be set. I think Sort subnets to avoid spurious diffs #5208 makes a few additions that will be useful here. However, I'd like to go through the option_settings and figure out which
    of those need some special handling to detect when there are changes.
  • CNAME swap functionality: http://docs.aws.amazon.com/elasticbeanstalk/latest/dg/using-features.CNAMESwap.html. See comments above for details on that.
  • Fix minor issue with the tests for these resources. Solution stack is hard coded into the test documents. This causes a problem if someone else wants to run
    the acceptence tests, because their AWS account may not have access to the same solution stacks. So this should be filled in dynamically, based on the account running
    the tests.

@tecnobrat
Copy link

I checked out this branch yesterday, and tested a simple workflow that we want to use. This branch seemed to work perfectly. Great work.

@collisdigital
Copy link

👍 we're using it too, working well, would love to see this on master now.

@ocxo
Copy link

ocxo commented Feb 24, 2016

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?

@dcosson
Copy link
Contributor

dcosson commented Feb 25, 2016

+1 I'd love to see this in the next release as well!

@ocxo
Copy link

ocxo commented Feb 26, 2016

Rebased against master on #5349 to help move this along. Feel free to use if it helps or discard.

@phinze
Copy link
Contributor

phinze commented Feb 26, 2016

Hiya folks - this is on our radar and we'll make sure it gets in before the next release! 👍

catsby and others added 8 commits March 7, 2016 11:22
* 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
  ...
…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.
@catsby
Copy link
Contributor Author

catsby commented Mar 7, 2016

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
@catsby
Copy link
Contributor Author

catsby commented Mar 7, 2016

I kind of lied – I didn't merge this branch as is, I squashed it into f0d3176

@Lifto
Copy link

Lifto commented Apr 15, 2016

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" {
application = "tf-test-name"
name = "tf-test-version-label"
bucket = "${aws_s3_bucket.default.id}"
key = "${aws_s3_bucket_object.default.id}"
}

Errors:

  • aws_elastic_beanstalk_application_version.default: Provider doesn't support resource: aws_elastic_beanstalk_application_version

@dharrisio
Copy link
Contributor

@Lifto Currently Terraform doesn't support the application version resource. This is something I am still working on, but ran into a few issues. You can track the progress on this in #5770.

@jen20 jen20 deleted the f-aws-elastic-beanstalk branch April 24, 2016 23:55
@rurreac
Copy link

rurreac commented May 4, 2016

My apologise if this is known already. I run into an issue with something like the following:

resource "aws_elastic_beanstalk_environment" "default" {
    name                = "${var.app_name}-${var.env}"
    application         = "${aws_elastic_beanstalk_application.default.name}"
    description         = "${var.app_description}"
    solution_stack_name = "${var.beanstalk_solution_stack}"

    setting {
        namespace = "aws:autoscaling:scheduledaction"
        name      = "DesiredCapacity"
        value     = "2"
    }
}

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

@dharrisio
Copy link
Contributor

@rurreac I was unaware of that issue, thanks for reporting it. I've created a separate issue to track it, as this one is closed. #6476

bmcustodio pushed a commit to bmcustodio/terraform that referenced this pull request Sep 26, 2017
@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.
@ghost
Copy link

ghost commented Apr 26, 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 26, 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.