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

Add Blue/Green Deployments to CodeDeploy DeploymentGroups. #11700

Closed

Conversation

niclic
Copy link
Contributor

@niclic niclic commented Feb 4, 2017

This PR has been migrated here: hashicorp/terraform-provider-aws#1162

TODO:

Changes to the aws_codedeploy_deployment_group resource:

  • Add deployment_style.
  • Add load_balancer_info.
  • Add blue_green_deployment_config.
  • Refactoring...
  • Update Documentation.
  • Add Tests for buildDeploymentStyle and deploymentStyleToMap
  • Consider prefixing all build/map/validation tests so none are missed.
  • Write a complete test for a blue green deployment scenario that includes all three resources.
  • Verify that the documented configurations are valid (DONE, tests not committed).
  • Consider adding validation checks for blue_green_deployment_config values.

@niclic
Copy link
Contributor Author

niclic commented Feb 18, 2017

I believe this PR is complete.

NOTES:

  • Every field on deployment_style, load_balancer_info, and blue_green_deployment_config is marked Optional, in keeping with the official docs and source. However, performing CRUD operations on these resources suggest that this is not actually the case.

Consider the following exceptions:

  • DEPLOYMENT STYLE

    • InvalidDeploymentStyleException: Deployment type or deployment option cannot be null or empty
  • LOAD BALANCER INFO

    • InvalidLoadBalancerInfoException: ELB information has to contain at least 1 ELB.

    • InvalidLoadBalancerInfoException: ELB has to have a name. Cannot be null or empty.

  • BLUE GREEN DEPLOYMENT CONFIG

    • InvalidBlueGreenDeploymentConfigurationException: Deployment ready option cannot be null for blue green deployment.

    • InvalidBlueGreenDeploymentConfigurationException: Terminate blue instances on deployment success behaviour cannot be set to null for Blue Green deployments.

However, until more information is available, I went with the official docs.

  • I'm not sure if the various validation functions are useful or not. If a new option is added to the sdk the corresponding validation needs to be updated to use this new functionality. This seems brittle.

  • There are rules about what options are valid together. For example, IN_PLACE deployment type is not supported with WITH_TRAFFIC_CONTROL option. I didn't encode these checks for the same reason (although I did mention this in the documentation). I don't think the point of terraform is to reproduce every nuance of the official SDK.

@niclic niclic changed the title WIP: Add Blue/Green Deployments to CodeDeploy DeploymentGroups. Add Blue/Green Deployments to CodeDeploy DeploymentGroups. Feb 18, 2017
@niclic
Copy link
Contributor Author

niclic commented Feb 18, 2017

For reference, here are some other encountered exceptions:

  • DEPLOYMENT STYLE

    • An error occurred (InvalidDeploymentStyleException) when calling the CreateDeploymentGroup operation: IN_PLACE deployment type not supported with WITH_TRAFFIC_CONTROL option.

    • An error occurred (InvalidDeploymentStyleException) when calling the CreateDeploymentGroup operation: BLUE_GREEN deployment type not supported with WITHOUT_TRAFFIC_CONTROL option.

  • LOAD BALANCER INFO

    • InvalidLoadBalancerInfoException: Only one ELB is supported per deployment group at this moment.

NOTE: The schema does not enforce this restriction.

  • BLUE GREEN DEPLOYMENT CONFIG
    • InvalidBlueGreenDeploymentConfigurationException: Exactly one AutoScaling group must be specifed when selecting the COPY_AUTO_SCALING_GROUP green fleet provisioning option.

    • InvalidBlueGreenDeploymentConfigurationException: Deployment ready action cannot be set to CONTINUE_DEPLOYMENT when timeout is specified.

@niclic niclic force-pushed the codedeploy-blue-green-deployments branch from 12e3571 to 2fc29b4 Compare March 18, 2017 20:51
@niclic
Copy link
Contributor Author

niclic commented Mar 18, 2017

Changes:

@stack72 This should be good to go.

@niclic
Copy link
Contributor Author

niclic commented Apr 7, 2017

Argh!

@niclic
Copy link
Contributor Author

niclic commented Apr 9, 2017

In light of the change in #13396, what is the preferred approach to documenting new features?

I've added several features to CodeDeploy Deployment Group, and this PR in particular contains a lot of new documentation.

I've tried to strike a balance between being informative and rote duplication of the AWS docs, but perhaps I've erred too much in favor of too much detail.

And it's not just documentation. I've asked this question before when adding validation functions (including in my comments for this PR). When options change (either new ones are added or worse, existing options are removed), terraform needs to be updated to maintain parity with the sdk. If changes are caught and merged quickly, then perhaps it's not much of an issue, but that doesn't seem to always be the case in my experience.

I've added several such validation functions in this PR (e.g. see validateDeploymentOption), but never feel comfortable doing it. I think I might have been trying to allow terraform to fail fast. But maintaining these functions can be hard, so it is a trade off.

Then again, I'm not sure how much of a problem this really is. If you enter an invalid option, that's on you.

Any feedback is welcome.

/cc @radeksimko

@philwilt
Copy link

philwilt commented Apr 21, 2017

@niclic this is the resolution of the merge conflict: niclic#1

niclic and others added 17 commits April 23, 2017 11:06
  - improved create test
  - added update test
  - added delete test, which displays unexpected behaviour
  - Not sure why this test does not pass as is.
  - After the update, I would expect to see no load_balancer_info.
  - don't check computed attribute values (they could change)
  - to make it easier to run them all together.
  - deployment_style
  - load_balancer_info
  - blue_green_deployment_config
  - cleanup
@niclic niclic force-pushed the codedeploy-blue-green-deployments branch from 0cbd4dd to b895964 Compare April 23, 2017 16:55
@niclic
Copy link
Contributor Author

niclic commented Apr 23, 2017

Okay, this has been rebased again to remove documentation conflicts.

Unfotunately, the deployment_style tests now fail for some reason, where they didn't before. Strangely, the blue_green_deployment_config tests do not fail for this reason, which doesn't immediately make any sense.

--- FAIL: TestAccAWSCodeDeployDeploymentGroup_deploymentStyle_create (41.50s)
  testing.go:280: Step 1 error: Error applying: 1 error(s) occurred:
    
    * aws_codedeploy_deployment_group.foo_group: 1 error(s) occurred:
    
    * aws_codedeploy_deployment_group.foo_group: InvalidLoadBalancerInfoException: ELB information has to contain at least 1 ELB.
      status code: 400, request id: 97f3be18-2844-11e7-becf-5566753218d9
FAIL
exit status 1

I will have to investigate.

@niclic
Copy link
Contributor Author

niclic commented Apr 23, 2017

Hmm. That was unexpected. All tests pass ^^^. And yet, I get (admittedly unexpected) failures locally.

I have found an issue which explains the test failures. This must have crept in somehow, perhaps through an update to the SDK. Even though the tests pass here, I am going to push another update.

@niclic
Copy link
Contributor Author

niclic commented Apr 23, 2017

When creating deployments that use traffic control:

  deployment_style {
    deployment_option = "WITH_TRAFFIC_CONTROL"
    deployment_type = "BLUE_GREEN"
  }

Then you MUST also create a load_balancer_info resource to specify the load balancer for traffic control. This makes sense. It's just not clear why the deployment_style tests passed to this point. I don't see any obvious change to the SDK or documentation. Having said that, the AWS Console enforces this requirement.

As noted above, there are a lot of implicit invariants in this new set of resources. This is one more.

@niclic
Copy link
Contributor Author

niclic commented Apr 23, 2017

I think we are good to go here. @stack72

@niclic
Copy link
Contributor Author

niclic commented May 10, 2017

Any reason why this can't be merged? @stack72

Looks like it would make many people happy. :)

@dev-alan-alves
Copy link

Pleaseeeeeeeee

@grubernaut grubernaut requested a review from radeksimko May 15, 2017 12:44
@zdobrenen
Copy link

Is there any word on the progress of merging this PR? Thanks!

@niclic
Copy link
Contributor Author

niclic commented Jun 6, 2017

@stack72 ^^^

@Hashfyre
Copy link

Hashfyre commented Jun 7, 2017

This is something we would like to integrate in our systems, much appreciated @niclic

@aredridel
Copy link

I also need this just for load_balancer_info support (for in-place deployments that take instances out of a load balancer while deploying)

@niclic
Copy link
Contributor Author

niclic commented Jul 15, 2017

This PR has been migrated here: hashicorp/terraform-provider-aws#1162

Please add your reactions of support to the new PR.

I will close this once the new PR has been merged. :)

@niclic
Copy link
Contributor Author

niclic commented Oct 12, 2017

This has been merged in hashicorp/terraform-provider-aws#1162.

Thank you to @radeksimko for all his help in getting this done!

@niclic niclic closed this Oct 12, 2017
@ghost
Copy link

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

7 participants