Migrating to new org and terraform registry #1

brandonjbjelland · 2017-09-27T23:09:34Z

Many small improvements made for this move and release:

Added

moved data sources to dedicated data.tf file.
aws_caller_identity now used to gather account_id rather than using a variable.
tests added for target_group and expanded for alb.

Changed

altered structure of module to conform to the new Terraform registry standards
principle_account_id (sp) moved to a data source rather than variable map. Spelling corrected.
removed redundant /test/alb directory which had module contents copied. Test kitchen now uses the module itself.
pinned examples to provider and terraform versions to harden versioning.
self signed cert added to the test fixtures, eliminating the need for manual upload and terraform.tfvars configuration of ssl cert arn.

…nded.

brandonjbjelland · 2017-10-03T02:26:56Z

Bumping the review here. I've still yet to test pulling this from the terraform registry (can't do that until it's published here and updated there) and I'm eager to test that functionality and make sure it aligns with the docs included.

brandonjbjelland · 2017-10-10T01:05:07Z

Bumping once more, hoping for review here. I have tests working elsewhere and would like to get it in this repo under that model as well but can't do that until this is merged in. @antonbabenko

antonbabenko · 2017-10-10T17:57:37Z

README.md

+* Internal IP ALBs
+* External IP ALBs
+
+It's recommended you use this module with


Link to the new/supported repositories: https://github.com/terraform-aws-modules/terraform-aws-security-group and https://github.com/terraform-aws-modules/terraform-aws-elb

Hmmm, I feel like it's a distinct and different task to alter all examples/tests to leverage the terraform registry modules. I'll do the work here but we should really strive for incremental changes where possible going forward.

antonbabenko · 2017-10-10T17:59:33Z

README.md

+documentation](https://aws.amazon.com/elasticloadbalancing/applicationloadbalancer/).
+For an example of using ALB with ECS look no further than the [hashicorp example](https://github.com/terraform-providers/terraform-provider-aws/blob/master/examples/ecs-alb).
+
+## Input Variables


Terraform registry is parsing variables defined in *.tf files, so there is no need to put it here and remember to update it every time when something is changed in them.

Will remove. Thanks!

antonbabenko · 2017-10-10T17:59:53Z

README.md

+* `tags` - A mapping of tags to assign to the resource.
+
+## Outputs
+* `alb_id` - `id` of the ALB created.


Same for outputs

antonbabenko · 2017-10-10T18:02:26Z

README.md

+  log_bucket          = "${var.log_bucket}"
+  log_prefix          = "${var.log_prefix}"
+  subnets             = "${var.public_subnets}"
+  vpc_id              = "${var.vpc_id}"


I usually put the highest-impact attributes at the top. In this case vpc_id and subnets at the most required.

Use complete values instead of references to data sources, var, other resources. This example should show exactly what is expected, but not how you will pass it into.

antonbabenko · 2017-10-10T18:02:55Z

README.md

+}
+```
+3. Always `terraform plan` to see your change before running `terraform apply`.
+4. Win the day!


Will do tomorrow! :)

antonbabenko · 2017-10-10T18:09:04Z

examples/test_fixtures/main.tf

+}
+
+provider "template" {
+  version = "1.0.0"


Do you actually need a specific version?

Make sure it works with the latest stable version.

Again moved to optimistic. Version constraints are important and will only become more important moving forward. I would only feel comfortable unsetting this if the module were actually tested against those previous versions.

antonbabenko · 2017-10-10T18:10:08Z

examples/test_fixtures/main.tf

+}
+
+module "sg_https_web" {
+  source              = "github.com/terraform-community-modules/tf_aws_sg//sg_https_only"


Use new repo for security group

antonbabenko · 2017-10-10T18:11:39Z

examples/test_fixtures/main.tf

+  }
+}
+
+module "vpc" {


Can you rely on default VPC which is already created? I use it in other examples and I think it will work for your example too.

I'd prefer to bring with my module an example that creates everything it needs. That way it's self-sufficient. Some default resources are inherently insecure (e.g. security groups) and I don't think it's a good idea to rely on them being in any particular state or existing at all especially since this is core to the test suite.

I'll be migrating this to the registry VPC module instead.

antonbabenko · 2017-10-10T18:16:04Z

test/integration/default/local_alb.rb

@@ -0,0 +1,60 @@
+require 'awspec'
+puts $:


What does this do? :)

I've deleted the puts. It was debugging info.

antonbabenko · 2017-10-10T18:28:16Z

variables.tf

+  type        = "list"
+}
+
+variable "aws_region" {


Should not be possible to customize in the module itself.

Unsure what's meant by this. Can you explain?

antonbabenko · 2017-10-10T18:28:34Z

variables.tf

+
+variable "alb_security_groups" {
+  description = "A comma separated string of security groups with which we associate the ALB. e.g. 'sg-edcd9784,sg-edcd9785'"
+  type        = "list"


So, list or string. Update description.

antonbabenko · 2017-10-10T18:28:58Z

variables.tf

+
+variable "alb_name" {
+  description = "The name of the ALB as will show in the AWS EC2 ELB console."
+  default     = "my-alb"


Don't make defaults for critical things like name.

antonbabenko · 2017-10-10T18:29:33Z

variables.tf

+*/
+
+variable "alb_is_internal" {
+  description = "Determines if the ALB is internal. Default: false"


Usually there is o need to write what is the default value, because it is visible 1 line below.

Removed where relevant.

antonbabenko · 2017-10-10T18:30:15Z

variables.tf

+}
+
+variable "alb_protocols" {
+  description = "A comma delimited list of the protocols the ALB accepts. e.g.: HTTPS"


Can you make it to accept list natively and join to string, if resource expect string.

antonbabenko · 2017-10-10T18:30:52Z

variables.tf

+
+variable "backend_port" {
+  description = "The port the service on the EC2 instances listen on."
+  default     = 80


No assumption. Make it fail, if user put bad value.

Again here, providing a default of what many if not most people would use (and a collection of defaults that work together) is not a bad approach. Following "make no assumptions " to a logical conclusion leaves a module consumer requiring to specify every last variable which strips some of the value of modules. Modules are intended to enable people to get started quickly with bundled resources. Part of that is adding defaults that are likely to fit many use cases.

antonbabenko · 2017-10-10T18:31:14Z

variables.tf

+
+variable "backend_protocol" {
+  description = "The protocol the backend service speaks. Options: HTTP, HTTPS, TCP, SSL (secure tcp)."
+  default     = "HTTP"


Same here, don't use default.

I don't completely follow the logic of not providing any defaults... sane and often used defaults are a good thing to offer as they are in part what adds value to modules. If literally every variable is stripped of defaults, the result is a module that is pretty well as long as specifying all the components by hand without a module.

Can you make a better case for not providing defaults like this?

antonbabenko · 2017-10-10T18:31:52Z

variables.tf

+}
+
+variable "cookie_duration" {
+  description = "If load balancer connection stickiness is desired, set this to the duration that cookie should be valid. If no stickiness is wanted, leave it blank. e.g.: 300"


blank, eg: 300. So what?

An example value that would work is "300". I've clarified the description.

antonbabenko · 2017-10-10T18:32:04Z

variables.tf

+
+variable "health_check_path" {
+  description = "The URL the ELB should use for health checks. e.g. /health"
+  default     = "/"


antonbabenko · 2017-10-10T18:33:03Z

outputs.tf

@@ -0,0 +1,24 @@
+output "alb_id" {


Put all outputs available from Terraform along with description which comes from documentation page.

Not sure what's meant by this. All outputs have descriptions and values. Can you clarify?

antonbabenko · 2017-10-10T18:35:01Z

Sorry for the delay in response.

brandonjbjelland · 2017-10-12T03:01:53Z

All feedback addressed locally and I'll push up the branch in the next couple hours. A few questions to your comments are above.

In the spirit of fairness I need a couple things if I'm going to keep maintaining the repo within this org:

I want to be listed as the maintainer of this project on the terraform registry. If that means we need to delete and resubmit alb or transfer ownership, let's do that. I started the project, have grown it, and I would like credit as its primary maintainer.
I need the ability to publish changes here (to github) and not be blocked by review just as you're not blocked on your repos. Probably just a permissions oversight. It doesn't really make sense for the primary maintainer (me) to not be allowed to actually own the repo and control its direction.

I hope you can agree and make those adjustments. I would guess were likely just wanting to get naming right and consistent for this set of AWS modules and that you're striving for quality, but it's only fair for me to be credited with my work and have the ability to continue it as the maintainer. Sound good? 🤝

antonbabenko · 2017-10-12T07:38:00Z

@brandoconnor I respect your contributions, ownership and completely agree with all you are saying. Also, small iterations are important, and we needed to make it fit the registry standards. This was mostly what I was trying to comment about. I will do my review during today and you can merge it when you want and publish to the registry.

After that let's review each other code as normal.

antonbabenko · 2017-10-12T07:40:49Z

README.md

+[terraform-aws-security-group](https://registry.terraform.io/modules/terraform-aws-modules/security-group/aws), and 
+[terraform-aws-autoscaling](https://registry.terraform.io/modules/terraform-aws-modules/autoscaling/aws/).
+
+## Why ALB instad of ELB?


antonbabenko · 2017-10-12T08:25:05Z

locals.tf

@@ -0,0 +1,18 @@
+locals {


I think this can be generated using data-source, or load from template file (similar to how you do with certificates) to be more consistent throught the code.

Can you find a doc or the source code for it? I looked but didn't see it.

https://www.terraform.io/docs/providers/aws/d/iam_policy_document.html - this is it

Hmmm so that's what I looked at but I thought an IAM policy was distinct from an S3 bucket policy. Turns out, they are different (S3 contains a principal and IAM doesn't) however that's an argument for this resource. That's all to say, if this can be used to generate more than IAM policy, it's probably badly named.

Will fix tonight.

I had exactly the same feeling when I first discovered that data source.

… in the next iteration.

github-actions · 2022-11-17T02:29:37Z

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

brandonjbjelland added 2 commits September 27, 2017 15:56

migrating to the terraform registry and new org. Tests fixed and expa…

406d7d1

…nded.

updated docs

46634e9

brandonjbjelland mentioned this pull request Sep 27, 2017

Migrate code to the terraform-aws-modules/terraform-aws-alb and publish in the registry terraform-community-modules/tf_aws_alb#14

Closed

antonbabenko self-requested a review September 28, 2017 18:39

brandonjbjelland added 2 commits September 28, 2017 14:35

Giving thanks to contributors in the changelog

a52a6a7

debugging rhcl failure

2af9897

antonbabenko reviewed Oct 10, 2017

View reviewed changes

PR #1 feedback addressed - see CHANGELOG

6a72c49

antonbabenko approved these changes Oct 12, 2017

View reviewed changes

brandonjbjelland added 3 commits October 19, 2017 17:37

fixed up the policy creation. tests not working atm

9e1857b

pre-commit config now in place

4ed1c87

environment variable for region needed to be set. Handing this better…

1452128

… in the next iteration.

brandonjbjelland self-assigned this Oct 20, 2017

brandonjbjelland merged commit b5a4c76 into terraform-aws-modules:master Oct 20, 2017

github-actions bot locked as resolved and limited conversation to collaborators Nov 17, 2022

Migrating to new org and terraform registry #1

Migrating to new org and terraform registry #1

Conversation

brandonjbjelland commented Sep 27, 2017

Added

Changed

brandonjbjelland commented Oct 3, 2017

brandonjbjelland commented Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonjbjelland Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonbabenko commented Oct 10, 2017

brandonjbjelland commented Oct 12, 2017 • edited Loading

antonbabenko commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 17, 2022

brandonjbjelland commented Oct 10, 2017 •

edited

Loading

brandonjbjelland Oct 12, 2017 •

edited

Loading

brandonjbjelland commented Oct 12, 2017 •

edited

Loading