#pr-reviews (2021-03)
Pull Request Reviews for Cloud Posse Projects
2021-03-01
Still says it needs to validate code owners. I saw the other day that someone was able to retrigger that check and it worked. Is there a gitops command for that?
Basically anytime the auto-format formats code, it causes the validate code owners to not run. We can fix it but it will require another mass update. We are trying to queue up some updates for that
In the meantime just make sure to auto format locally to avoid any issues
Thanks. I pushed an empty commit for now https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/127
2021-03-02
review plz https://github.com/cloudposse/terraform-aws-ecs-container-definition/pull/126
Run make init
make init
shell: bash -e -o pipefail {0}
env:
MAKE_INCLUDES: Makefile
BUILD_HARNESS_BRANCH: master
curl: (6) Could not resolve host: git.io
make: *** No rule to make target ‘init’. Stop.
Error: Process completed with exit code 2.
the readme
is not working for some reason
nothing is returned from https://git.io/build-harness
DNS_PROBE_FINISHED_NXDOMAIN
$ curl -vvv <https://git.io/build-harness>
* Could not resolve host: git.io
* Closing connection 0
curl: (6) Could not resolve host: git.io
so this is what happens when git.io is down
well I guess we will have to wait
looks like it’s back @jose.amengual
i did a /rebuild-readme
but it doesnt look like it’s running. last fail was from an hour ago
the emojis are on my comment…
weird
i just ran a /test all and i got a thumbs up emoji but tests aren’t rerunning
I’m running it manually to see
the tests look like they kicked off!
https://github.com/cloudposse/terraform-aws-rds/pull/109 Not sure why the do not merge label was added without any comments
2021-03-08
https://github.com/cloudposse/terraform-aws-ecs-container-definition/pull/124 Can someone please take a look at this one? Thanks!
@Frank merged
Thanks!
2021-03-09
https://github.com/cloudposse/terraform-aws-elasticache-redis/pull/115 <– the 0.35.0 version causes quite a few problems in existing setups. This PR should provide a way to work around that.
hello! looking for a review pretty please https://github.com/cloudposse/terraform-aws-elasticache-redis/pull/93
2021-03-12
hey all, I am a bit confused what is needed for this pull request:
comment on the PR to get more details
hello again, here’s one adding more utility to the CDN by passing cache policy ID https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/57
this is similar, needs tests started https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/140
2021-03-15
hey all, to proceed pull request 73, I rebased:
could you have a look ?
looks like is still behind master
ran a fmt through all the terraform code in the branch
and pushed
should I allow the cloudpossebot write access to my repo ?
yes
you can pull from origin/master and fix the conflicts
the cloudposse master ?
no the master branch
your code is behind so you need to rebase
ok, let me check
thx for the help, I don’t do this often
2021-03-16
Hi all, found the time to do this properly. This is a rebase of pull request 73:
I left a comment
let me check, thanks
pushed a change, is this what you mean ?
left another comment but basically you can do : for_each = var.secondary_sources
yeah that’s clear, thx
pushed
have you look at this ? https://github.com/cloudposse/terraform-aws-codebuild/pull/78
yeah I saw this as well
it could go hand in hand indeed
let me check
secondary_sources is missing a description
the tests are failing on that
let me check
where should that go ?
you need a description in every variable
ha correct, let me fix it
pushed, I am not a native English speaker, so feel free to correct
me neither
what do you mean by (Optional) secondary source for the built next to the primary location defined in variable source_location
built or build?
to build
I will rephrase, second
ok
rephrased and pushed
so it is an additional source for the build
?
yes correct
so you can use two s3 buckets for example, sepparate from each other
we do use it as such
(Optional) secondary source for the codebuild project in addition to the primary location
agreed
that is more concise
agreed
please change it
done
merged
excellent !
thx man !
np
Could you have a look ?
A very big thank you to @jose.amengual for helping me with merging code ! You are one of kind !
here goes the other one:
make sure to run make init
make readme
closed in favour of #77
Hi all, since there have been a few changes on this module lately, can I get a final review of this one too? https://github.com/cloudposse/terraform-aws-codebuild/pull/78
Thanks @jose.amengual!
np
@Joe Hosteny have you seen this :
resource/aws_codebuild_project: The source and secondary_sources configuration block auth attributes have been deprecated to match the CodeBuild API documentation. Use the aws_codebuild_source_credential resource instead. (#17465)
I have not. We are only using the secondary artifact config, which I don’t think this applies to?
ohh I think it was @Bart Coddens
you both have PRs for the repo
I could help look into this one, but I will be on vacation this week
np
2021-03-17
2021-03-18
2021-03-19
2021-03-21
2021-03-23
2021-03-24
Can someone please take a look at this one: https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/pull/112 ?
Have to get a core team member to review this one. Will start that process.
Could i please get a review for this ^
All 5 of the open PRs for the asg module are also ready for review
https://github.com/cloudposse/terraform-aws-ec2-autoscale-group/pulls
2021-03-25
2021-03-26
could I get a review on this https://github.com/cloudposse/terraform-aws-ecs-container-definition/pull/131
I got some quota left — particularly for small changes like that one
Though you need to post an empty commit to re-trigger some failed CI @RB
i think i messed it up
but now it’s running all the tests again lol
Haha as long as it runs that one test then you’re good.
this guy AGAIN!!!!!!
lol
i saw your comment here https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/131#issuecomment-808486904
the last update on the readme was in line with the automation, no ?
he updated the yaml
mmm no, he needs to only update the yaml
the then run make readme
wouldn’t it be the same result ?
if i ran /rebuild-readme wouldnt it be enough ?
it will fail I think
but the reason behind running the make command is that the readme itself gets updated with the latest too
that makes sense!
2021-03-27
this was even more of a rabbit hole! which lead to this PR #143
Could i get another review on the removal of the static s3 bucket hack?
OMG that is confusing
the workaround of the data sources is interesting
2021-03-28
Fully removes the template provider from versions.tf. i forgot about this file in my last pr
https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/146
2021-03-29
2021-03-30
Can someone please take a look at this one? Thanks! https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/147
another good one from Harry https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/60 thanks in advance!
can i get a review on https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/143
cc: @Andriy Knysh (Cloud Posse) if time permits since you left a comment on it last
Very small pr
One more small pr https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/142
2021-03-31
You folks are probably getting tired of me. Almost done with all the prs in cloudfront cdn!
https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/149
test failing