#pr-reviews (2023-01)
Pull Request Reviews for Cloud Posse Projects
2023-01-03
![Bart Coddens avatar](https://secure.gravatar.com/avatar/a61940c9d1f90f24a5ab085de26797bc.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
what
Upgrade the module versions to recent versions:
cloudposse/security-group/aws = 2.0.0
cloudposse/route53-cluster-hostname/aws = 0.12.3
why
To stay in sync with security fixes in the upstream modules
references
closes #117
![Bart Coddens avatar](https://secure.gravatar.com/avatar/a61940c9d1f90f24a5ab085de26797bc.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
Simple upgrade, could someone have a look ?
2023-01-11
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
pretty please https://github.com/cloudposse/terraform-aws-alb/pull/127
what
• To support other type of load_balancing_algorithm_type (other than the default round_robin
)
why
• My use case requires to use least_outstanding_requests
type
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
what
• Fix bug for #100 • Add support for instance reuse policy for warm pool
why
• My module needs this new feature for warm pool
references
2023-01-12
2023-01-16
![Frank avatar](https://secure.gravatar.com/avatar/1b4b7744d083431522fb3e1b49206492.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
add proper description, fix conflicts and then we can merge it
![Frank avatar](https://secure.gravatar.com/avatar/1b4b7744d083431522fb3e1b49206492.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
It’s not my PR (but @Ahmed Kamal, assuming this is the same persoon) so unfortunately so I cannot do that.
I can however create a new PR if he is unable to do so.
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
yes, go ahead, that PR is very old
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
you can mention him in the PR to give him credit
![Frank avatar](https://secure.gravatar.com/avatar/1b4b7744d083431522fb3e1b49206492.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
what
• Adds http_version
variable.
why
• To be able to control the maximum HTTP version to be supported by the CloudFront Distribution, defaults to http2
references
2023-01-30
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
quick one - if someone can start the tests https://github.com/cloudposse/terraform-aws-ec2-autoscale-group/pull/102
what
• Current module only support statistic
, not extended_statistic
for custom_alarms
why
• extended_statistic
is useful for metric TargetResponseTime
references
• Link to any supporting github issues or helpful documentation to add some context (e.g. stackoverflow).
• Use closes #123
, if this PR closes a GitHub issue #123
![Sergey avatar](https://secure.gravatar.com/avatar/447699c4c807472d68c9996e58609bc5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
Hello. I’m using the “terraform-aws-cloudtrail-s3-bucket” module, but it lacks 2 variables (object_lock_configuration & bucket_key_enabled), I added them to the module: 1) https://github.com/cloudposse/terraform-aws-cloudtrail-s3-bucket/pull/69
2) https://github.com/cloudposse/terraform-aws-cloudtrail-s3-bucket/pull/70
Since the module refers to “terraform-aws-s3-log-storage”, I had to add a variable in it: 3) https://github.com/cloudposse/terraform-aws-s3-log-storage/pull/84
Note that the final module “cloudposse/s3-bucket/aws” already has these variables. Please approve these changes as soon as possible, the code is tested and works.
![Sergey avatar](https://secure.gravatar.com/avatar/447699c4c807472d68c9996e58609bc5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
Hi, why is my message being ignored?
![Sergey avatar](https://secure.gravatar.com/avatar/447699c4c807472d68c9996e58609bc5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
@jose.amengual can you check it and merge?
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
People are busy, no one is ignoring you
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
tests are no passing
![Sergey avatar](https://secure.gravatar.com/avatar/447699c4c807472d68c9996e58609bc5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
@jose.amengual Hello, the error in the “terraform-aws-cloudtrail-s3-bucket” module occurs because it depends on the “terraform-aws-s3-log-storage” module. First you need to accept PR 3) https://github.com/cloudposse/terraform-aws-s3-log-storage/pull/84
then the errors will disappear in: 1) https://github.com/cloudposse/terraform-aws-cloudtrail-s3-bucket/pull/69 2) https://github.com/cloudposse/terraform-aws-cloudtrail-s3-bucket/pull/70
I checked the code, it works.
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
# 84 is good to go and merged
![Sergey avatar](https://secure.gravatar.com/avatar/447699c4c807472d68c9996e58609bc5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
Great news, thanks! Now you can run tests in 69 and 70 in parallel.
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
you will need to update the module version
![Sergey avatar](https://secure.gravatar.com/avatar/447699c4c807472d68c9996e58609bc5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
I don’t know how, I just added variables.
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
that line with the new version of log storage module
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
that is what I mean
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
to 1.1.0
in this case
![Sergey avatar](https://secure.gravatar.com/avatar/447699c4c807472d68c9996e58609bc5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
Okay, I’ll do it and report back. Please check #70 it should not be version dependent.
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
#70 is failing
![Sergey avatar](https://secure.gravatar.com/avatar/447699c4c807472d68c9996e58609bc5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
Ok, I remove argument named “nullable”.
![Sergey avatar](https://secure.gravatar.com/avatar/447699c4c807472d68c9996e58609bc5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
This is strange, because I did this variable in the same way https://github.com/cloudposse/terraform-aws-s3-log-storage/blob/master/variables.tf#L86
nullable = false
![Sergey avatar](https://secure.gravatar.com/avatar/447699c4c807472d68c9996e58609bc5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
more issues
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
this could be tricky
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
since the modules was updated then it requires the a new provider version
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
since is using and old version that does not support moved, etc
![Sergey avatar](https://secure.gravatar.com/avatar/447699c4c807472d68c9996e58609bc5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
omg, I just need support for two variables and I added 2 lines of code. I am not an OPS qualified and cannot understand all the processes.
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
welcome to Terraform modules
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
![Sergey avatar](https://secure.gravatar.com/avatar/447699c4c807472d68c9996e58609bc5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
I updated https://github.com/cloudposse/terraform-aws-cloudtrail-s3-bucket/blob/master/versions.tf according to module https://github.com/cloudposse/terraform-aws-s3-log-storage/blob/master/versions.tf.
@jose.amengual Can you please tell me how can I run the “test /all” command for self-test?
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
only mantainers/contributors can run test all, you can’t
![Sergey avatar](https://secure.gravatar.com/avatar/447699c4c807472d68c9996e58609bc5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
What if the main code works correctly, but the code that executes it does not work correctly? Do I need to change the example code as well?)
The file https://github.com/cloudposse/terraform-aws-cloudtrail-s3-bucket/blob/master/examples/complete/versions.tf also had outdated versions.
ps I have this code inside terragrunt works fine
terraform {
required_version = ">= 0.13.0"
required_providers {
aws = {
source = "hashicorp/aws"
version = ">= 2.0"
}
local = {
source = "hashicorp/local"
version = ">= 1.2"
}
null = {
source = "hashicorp/null"
version = ">= 2.0"
}
}
}
![Sergey avatar](https://secure.gravatar.com/avatar/447699c4c807472d68c9996e58609bc5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
Can you run “test /all” again please?
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
done
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
I think you need to update this : https://github.com/ramses999/terraform-aws-cloudtrail-s3-bucket/blob/bucket_key_enabled/main.tf#L46
module "s3_access_log_bucket" {
![Sergey avatar](https://secure.gravatar.com/avatar/447699c4c807472d68c9996e58609bc5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
Other topic
Hello, I have fixed the bug https://github.com/cloudposse/terraform-aws-cloudwatch-logs/pull/38 Please review as soon as possible. The code has been tested and works.