#pr-reviews (2023-01)
Pull Request Reviews for Cloud Posse Projects
2023-01-03
Simple upgrade, could someone have a look ?
2023-01-11
2023-01-12
2023-01-16
someone will have to take it over
add proper description, fix conflicts and then we can merge it
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.
yes, go ahead, that PR is very old
you can mention him in the PR to give him credit
2023-01-30
quick one - if someone can start the tests https://github.com/cloudposse/terraform-aws-ec2-autoscale-group/pull/102
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.
Hi, why is my message being ignored?
@jose.amengual can you check it and merge?
People are busy, no one is ignoring you
tests are no passing
@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.
# 84 is good to go and merged
Great news, thanks! Now you can run tests in 69 and 70 in parallel.
you will need to update the module version
I don’t know how, I just added variables.
that line with the new version of log storage module
that is what I mean
to 1.1.0 in this case
Okay, I’ll do it and report back. Please check #70 it should not be version dependent.
#70 is failing
Ok, I remove argument named “nullable”.
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
more issues
this could be tricky
since the modules was updated then it requires the a new provider version
since is using and old version that does not support moved, etc
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.
welcome to Terraform modules
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?
only mantainers/contributors can run test all, you can’t
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
Can you run “test /all” again please?
done
I think you need to update this : https://github.com/ramses999/terraform-aws-cloudtrail-s3-bucket/blob/bucket_key_enabled/main.tf#L46
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.