#pr-reviews (2020-12)

Pull Request Reviews for Cloud Posse Projects

2020-12-23

2020-12-22

Alex Taylor avatar
Alex Taylor

Hello, another PR here for you (Allow configuring SCRAM authentication for MSK): https://github.com/cloudposse/terraform-aws-msk-apache-kafka-cluster/pull/6

Add scram authentication by taylora433 · Pull Request #6 · cloudposse/terraform-aws-msk-apache-kafka-cluster

what Allow cluster to be created with SCRAM authentication. why TLS authentication is not suitable in our use case, we need to use the SCRAM authentication mechanism.

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@Maxim Mironenko (Cloud Posse)

Add scram authentication by taylora433 · Pull Request #6 · cloudposse/terraform-aws-msk-apache-kafka-cluster

what Allow cluster to be created with SCRAM authentication. why TLS authentication is not suitable in our use case, we need to use the SCRAM authentication mechanism.

Alex Taylor avatar
Alex Taylor

The tests on this one appear to be broken due to an unsupported Kafka version? I haven’t changed anything to do with the versioning in this PR.

2020-12-21

Maxim Mironenko (Cloud Posse) avatar
Maxim Mironenko (Cloud Posse)

@Jeremy (Cloud Posse) Please, fix these repos with proper codeowners, when you have time: <https://github.com/cloudposse/terraform-aws-documentdb-cluster/pull/20> <https://github.com/cloudposse/terraform-aws-ec2-instance-group/pull/18> <https://github.com/cloudposse/terraform-aws-ec2-autoscale-group/pull/39> <https://github.com/cloudposse/terraform-aws-vpn-connection/pull/9>

1
1
Evangelos Karvounis avatar
Evangelos Karvounis

hello there! can someone review this PR (https://github.com/cloudposse/terraform-aws-ecs-container-definition/pull/107) please? cheers

1
Evangelos Karvounis avatar
Evangelos Karvounis

@ @Joe Niland

Makeshift (Connor Bell) avatar
Makeshift (Connor Bell)

Can do if nobody has gotten to it this evening (~8h from now). Just travelling back from holiday :)

Evangelos Karvounis avatar
Evangelos Karvounis

sure! safe travel

2020-12-19

Maxim Mironenko (Cloud Posse) avatar
Maxim Mironenko (Cloud Posse)

@Jeremy (Cloud Posse) Please, fix this repo with proper codeowners, when you have time: <https://github.com/cloudposse/terraform-aws-vpc-peering/pull/28>

1
Maxim Mironenko (Cloud Posse) avatar
Maxim Mironenko (Cloud Posse)

@Jeremy (Cloud Posse) and this one, please: <https://github.com/cloudposse/terraform-aws-cloudformation-stack/pull/8>

1
Maxim Mironenko (Cloud Posse) avatar
Maxim Mironenko (Cloud Posse)

@Jeremy (Cloud Posse) and this, please: <https://github.com/cloudposse/terraform-aws-s3-website/pull/46>

1
David avatar
David

First time submitting a PR to one of these repos. Would be grateful for any feedback to make this more in line with expectations. Hoping this is an easy merge. https://github.com/cloudposse/terraform-aws-s3-bucket/pull/66

Allow current object expiration to be disabled. by davidski · Pull Request #66 · cloudposse/terraform-aws-s3-bucket

what Adds a new option - &quot;enable_current_object_expiration&quot; for selectively disabling current object expiration. Default to true to maintain behavior with existing versions. why Users tha…

Joe Niland avatar
Joe Niland

LGTM except for one typo. We need to run the tests on it but GitHub Actions is currently having some issues. Hopefully resolved soon.

Allow current object expiration to be disabled. by davidski · Pull Request #66 · cloudposse/terraform-aws-s3-bucket

what Adds a new option - &quot;enable_current_object_expiration&quot; for selectively disabling current object expiration. Default to true to maintain behavior with existing versions. why Users tha…

party_parrot1
Joe Niland avatar
Joe Niland

@Jeremy (Cloud Posse) if still available, I think this one needs the Codeowners validation fix: https://github.com/cloudposse/terraform-aws-ec2-bastion-server/pull/49

sg: make ingress blocks optional, add egress block by joe-niland · Pull Request #49 · cloudposse/terraform-aws-ec2-bastion-server

&#39;Forked&#39; from #29 what Resurrect abandoned PR #29 created by @DeividasJackus Tweaked aws_security_group definition to: Only create ingress rules if allowed_cidr_blocks variable is specifie…

1
Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

Fixed

sg: make ingress blocks optional, add egress block by joe-niland · Pull Request #49 · cloudposse/terraform-aws-ec2-bastion-server

&#39;Forked&#39; from #29 what Resurrect abandoned PR #29 created by @DeividasJackus Tweaked aws_security_group definition to: Only create ingress rules if allowed_cidr_blocks variable is specifie…

Joe Niland avatar
Joe Niland

Thank you!

2020-12-17

Guy Elia avatar
Guy Elia

Hey Cloudposse, someone can review @Maxim Mironenko (Cloud Posse) PR? https://github.com/cloudposse/terraform-aws-rds/pull/81

Terraform 0.14 upgrade by maximmi · Pull Request #81 · cloudposse/terraform-aws-rds

what Upgrade to support Terraform 0.14 and bring up to current Cloud Posse standard why Support Terraform 0.14

1
Guy Elia avatar
Guy Elia

Thanks! @Joe Niland

Terraform 0.14 upgrade by maximmi · Pull Request #81 · cloudposse/terraform-aws-rds

what Upgrade to support Terraform 0.14 and bring up to current Cloud Posse standard why Support Terraform 0.14

Joe Niland avatar
Joe Niland

Np

loren avatar
loren
Pull request auto-merge public beta - GitHub Changelog attachment image

Pull request auto-merge public beta

2
1
Matt Gowie avatar
Matt Gowie

Ah but it’s a manual step like “okay, yeah merge this once those checks pass” not a “merge this if checks pass and it has this label”.

Pull request auto-merge public beta - GitHub Changelog attachment image

Pull request auto-merge public beta

Matt Gowie avatar
Matt Gowie

So mergify is still very relevant.

1
loren avatar
loren

yeah, not so great. basically just the “merge when checks pass” that gitlab has had for a while

1

2020-12-16

Maxim Mironenko (Cloud Posse) avatar
Maxim Mironenko (Cloud Posse)
Terraform 0.14 upgrade by maximmi · Pull Request #81 · cloudposse/terraform-aws-rds

what Upgrade to support Terraform 0.14 and bring up to current Cloud Posse standard why Support Terraform 0.14

Terraform 0.14 upgrade by maximmi · Pull Request #21 · cloudposse/terraform-tls-ssh-key-pair

what Upgrade to support Terraform 0.14 and bring up to current Cloud Posse standard why Support Terraform 0.14

Upgrade to support Terraform 0.14 by maximmi · Pull Request #10 · cloudposse/terraform-aws-ecs-cloudwatch-autoscaling

what Upgrade to support Terraform 0.14 and bring up to current Cloud Posse standard why Support Terraform 0.14

1
Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

Done

Terraform 0.14 upgrade by maximmi · Pull Request #81 · cloudposse/terraform-aws-rds

what Upgrade to support Terraform 0.14 and bring up to current Cloud Posse standard why Support Terraform 0.14

Terraform 0.14 upgrade by maximmi · Pull Request #21 · cloudposse/terraform-tls-ssh-key-pair

what Upgrade to support Terraform 0.14 and bring up to current Cloud Posse standard why Support Terraform 0.14

Upgrade to support Terraform 0.14 by maximmi · Pull Request #10 · cloudposse/terraform-aws-ecs-cloudwatch-autoscaling

what Upgrade to support Terraform 0.14 and bring up to current Cloud Posse standard why Support Terraform 0.14

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

Announcement: We have created some tools to help update our Terraform modules so that they work with Terraform 0.14. Please read the instructions at https://docs.cloudposse.com/community/updating-modules-for-terraform-14/ if you want to help by converting a module.

2

2020-12-15

Guy Elia avatar
Guy Elia

Hey guys, tiny PR for enabling terraform-aws-rds to work with terraform 14 https://github.com/cloudposse/terraform-aws-rds/pull/80

Upgrade dependency modules for support tf14 by guyelia · Pull Request #80 · cloudposse/terraform-aws-rds

what Upgrading dependency modules to versions that supporting Terraform 14 why For using this module with Terraform 14

Guy Elia avatar
Guy Elia

Hi, I am trying to mask sensitive information from plan and apply output. I tried couple of ways: • using sensitive keyword, but https://github.com/cloudposse/terraform-aws-rds-cluster.git?ref=tags/0.35.0 version pinned at required_version = “>= 0.12.0, < 0.14.0” and sensitive keyword is available from >= 0.14.0 • Tfmask doesn’t seems to work with resources or values which are lists. (I am trying to mask helm values variable). Any suggestions on how to go about this.

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@Maxim Mironenko (Cloud Posse)

Guy Elia avatar
Guy Elia

hey @Maxim Mironenko (Cloud Posse) should i close my PR?

Maxim Mironenko (Cloud Posse) avatar
Maxim Mironenko (Cloud Posse)

@ yes, I made a new one based on your PR, but because your branch was closed to writing I have to create new PR.

1
Tyrone Meijn avatar
Tyrone Meijn

Hey y’all, can somebody take a look at this one: https://github.com/cloudposse/terraform-aws-ec2-bastion-server/pull/45

I was trying to fix it locally and submit a PR before I saw this one and it looks good to me!

Conditionally configure security group ingress by alexandrusavin · Pull Request #45 · cloudposse/terraform-aws-ec2-bastion-server

what Conditionally configure default security group ingress only when length(var.ingress_security_groups) > 0 why Otherwise, it detects an update on every plan run even if applied (which who…

2020-12-13

2020-12-11

mihai avatar
mihai

hey hey, requesting a test run and quick review on https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/116

Add cloudfront origin identity iam arn output by bkero · Pull Request #116 · cloudposse/terraform-aws-cloudfront-s3-cdn

This adds an output to access the Cloudfront distribution&#39;s Origin Access identity, if it is enabled, else a blank string. We need this to provide access to the Cloudfront distribution from an …

1
Matt Gowie avatar
Matt Gowie

Merged and released as 0.38.0 — Thanks @!

Add cloudfront origin identity iam arn output by bkero · Pull Request #116 · cloudposse/terraform-aws-cloudfront-s3-cdn

This adds an output to access the Cloudfront distribution&#39;s Origin Access identity, if it is enabled, else a blank string. We need this to provide access to the Cloudfront distribution from an …

1
mihai avatar
mihai

perfect timing, thank you

1

2020-12-10

2020-12-09

Alex Taylor avatar
Alex Taylor

Hello I have a PR for you. This adds a delivery policy to an SNS topic (to override the default). I’m still new to making PRs here so please tell me if I’m doing something wrong https://github.com/cloudposse/terraform-aws-sns-topic/pull/23

Adds delivery policy for the SNS topic input as JSON by taylora433 · Pull Request #23 · cloudposse/terraform-aws-sns-topic

what Add a retry policy for the SNS topic why Override the default retry policy for SNS as some of the default settings are not suitable for all subscriptions NOTE: The value defaults to null, …

jose.amengual avatar
jose.amengual

please look at your fork permissions

Adds delivery policy for the SNS topic input as JSON by taylora433 · Pull Request #23 · cloudposse/terraform-aws-sns-topic

what Add a retry policy for the SNS topic why Override the default retry policy for SNS as some of the default settings are not suitable for all subscriptions NOTE: The value defaults to null, …

jose.amengual avatar
jose.amengual

it seems that is restricting the merge

Alex Taylor avatar
Alex Taylor

I don’t recall needing to change any permissions the last time I forked and made a PR. Do you know what settings are needed?

jose.amengual avatar
jose.amengual

nevermind, I think this is in our side

1

2020-12-08

Mikhail Naletov avatar
Mikhail Naletov
Use context.tf and latest null-label module by okgolove · Pull Request #9 · cloudposse/terraform-aws-ecs-cloudwatch-autoscaling

what Use context feature and latest null-label module why The module should have context feature due to it is used by another modules like ecs-web-app

Add customisable aws log prefix by okgolove · Pull Request #79 · cloudposse/terraform-aws-ecs-web-app

what Customisable aws log prefix why May be helpful in case of you don’t use name in label module

Matt Gowie avatar
Matt Gowie

Hey @ — we just made a change to our bats tests to enforce Terraform registry modules, so they may get hung up by this new requirement. We’re looking to have a make target to make this easy in the coming days.

Use context.tf and latest null-label module by okgolove · Pull Request #9 · cloudposse/terraform-aws-ecs-cloudwatch-autoscaling

what Use context feature and latest null-label module why The module should have context feature due to it is used by another modules like ecs-web-app

Add customisable aws log prefix by okgolove · Pull Request #79 · cloudposse/terraform-aws-ecs-web-app

what Customisable aws log prefix why May be helpful in case of you don’t use name in label module

Mikhail Naletov avatar
Mikhail Naletov

Nice improvement!

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@here PSA: we’re working on some of the underlying scaffolding and tooling to better support 0.14 and future updates. 0.13 was a big pain, and we learned a lot. A few things are happening behind the scenes:

• We’re switching everything on our side over to the terraform registry notation so we can use rennovatebot (which doesn’t like our ref=tags/... format). • In order to support the registry notation, we needed to update our test-harness to support bats (this is done, but is not backwards compatible). • We’re adding mergify to quickly merge automated PRs, but were quickly blocked by the next hurdle: mergify cannot be a CODEOWNER because it’s a GitHub App. So we need to upgrade our account. Working on that (but it’s expensive!) • We’ve added added the make targets to quickly convert a module to use the “new” (but not so new) provider and registry notation. But turns out many have trouble running this with the build-harness natively, so we’re going to add a make docker/shell target to run it in a container and mount the cwd into the container. This will also help with make readme and other things like it. • We’ve added the github actions to automatically rebuild the README nightly, but it’s blocked on the mergify issue above. • We’ve added the github actions to automatically update the [context.tf](http://context.tf) from the central copy in terraform-null-label • We’ve added the make target to the build-harness which will update the lower-bound pinning for modules pinned to >= 0.12 to be >=0.12.26 (to support new provider syntax)

• We’ve drafted the rennovatebot configuration to automatically update module pinning and run tests, then merge when they pass. All of this work will make all future upgrades of terraform breezy. Unfortunately, with so many changes, we ran into the inevitable rough edges.

This is all to say, we’re currently blocked on testing PRs because tests are failing due to our changes. We’re working to fix those. ETA is by end-of-the-week.

4
1
1
loren avatar
loren

i haven’t used mergify with CODEOWNERS… that’s interesting… i did find that if i enabled “restrict who can push to matching branches” in the branch protection rule, then i needed to add mergify there…

loren avatar
loren
05:17:21 PM
Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

note, I don’t think mergify is the account of the GitHub App “Mergify”

http://github.com/mergify

mergify - Overview

GitHub is where mergify builds software.

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

so with branch protections, you were able to add the mergify app?

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@Jeremy (Cloud Posse)

loren avatar
loren

yes, mergify is the bot account for the mergify app

loren avatar
loren

in the search box for that branch protection feature, it will only let you select users or teams that have write privileges to the repo, or app integrations

Jurgen avatar
Jurgen

you guys sound busy.

Jurgen avatar
Jurgen

how is all of this going?

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

very close! @Jeremy (Cloud Posse) just merged one more PR a few minutes ago related to the build-harness. The test-harness changes should merge any minute now.

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

I think we’re just about ready to start rolling it out wholesale (but defer to @Jeremy (Cloud Posse))

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

Build harness and Test harness updates have been released, now verifying that all is good.

1
Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

We will have a document describing how to update a Terraform module for Terraform 0.14 and our new standards soon.

Amit Karpe avatar
Amit Karpe

Can someone please let me know if this PR is good enough? This is my first PR, so if something is missing please educate me. https://github.com/cloudposse/terraform-aws-rds/pull/79

Added Microsoft SQL Server example by amitkarpe · Pull Request #79 · cloudposse/terraform-aws-rds

what Added Microsoft SQL Server example why There was MySQL RDS example but nothing for MSSQL

Joe Niland avatar
Joe Niland

I already commented on it earlier

Added Microsoft SQL Server example by amitkarpe · Pull Request #79 · cloudposse/terraform-aws-rds

what Added Microsoft SQL Server example why There was MySQL RDS example but nothing for MSSQL

Amit Karpe avatar
Amit Karpe

Thank you!!! Roughly how much time it will take?

I had one more PR on my mind - example for postgresql Should I do it? or Wait for automated tests results?

Joe Niland avatar
Joe Niland

Sorry, not sure exactly but possibly end of this week.

Joe Niland avatar
Joe Niland

Sure go ahead and submit it

1
Amit Karpe avatar
Amit Karpe

Can someone review the pr? Thanks

Amit Karpe avatar
Amit Karpe

@Joe Niland

I can still see the message “Merge is blocked which needs at least one approval.” How I can merge into master?

I think I don’t have permission to merge into master. Is my understanding correct?

Amit Karpe avatar
Amit Karpe

@Joe Niland I have done development on remote branch i.e. https://github.com/amitkarpe/terraform-aws-rds/tree/examples/mssql-complete/examples Based on my knowledge rebase to master for open source project is not good suggestion.

Can you let me know how should I do rebase?

amitkarpe/terraform-aws-rds

Terraform module to provision AWS RDS instances. Contribute to amitkarpe/terraform-aws-rds development by creating an account on GitHub.

Amit Karpe avatar
Amit Karpe
The golden rule of git rebase is to never use it on public branches.

The Golden Rule of Rebasing

So please let me know how to do it.

Merging vs. Rebasing | Atlassian Git Tutorial

Compare git rebase with the related git merge command and identify all of the potential opportunities to incorporate rebasing into the typical Git workflow

Joe Niland avatar
Joe Niland

I think all you need to do is merge master from the cloudposse repo into your feature branch.

Amit Karpe avatar
Amit Karpe

I am sorry but I am not sure how to do it. Here my following commands from my repo directory

 ➜ gco examples/mssql-complete
Switched to branch 'examples/mssql-complete'
Your branch is behind 'origin/examples/mssql-complete' by 2 commits, and can be fast-forwarded.
  (use "git pull" to update your local branch)

 ➜ git status
On branch examples/mssql-complete
Your branch is behind 'origin/examples/mssql-complete' by 2 commits, and can be fast-forwarded.
  (use "git pull" to update your local branch)

nothing to commit, working tree clean

 ➜ git merge master
Already up to date.

 ➜ gco master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.

 ➜ git merge master
Already up to date.
Amit Karpe avatar
Amit Karpe

Any suggestion?

Joe Niland avatar
Joe Niland

You need to do

git remote add upstream https://github.com/cloudposse/terraform-aws-rds.git
git fetch upstream
git merge upstream/master
Amit Karpe avatar
Amit Karpe

done

 ➜ git remote add upstream <https://github.com/cloudposse/terraform-aws-rds.git>

 ➜ git remote -v
origin	<https://github.com/amitkarpe/terraform-aws-rds> (fetch)
origin	<https://github.com/amitkarpe/terraform-aws-rds> (push)
upstream	<https://github.com/cloudposse/terraform-aws-rds.git> (fetch)
upstream	<https://github.com/cloudposse/terraform-aws-rds.git> (push)

 ➜ git fetch upstream
remote: Enumerating objects: 31, done.
remote: Counting objects: 100% (31/31), done.
remote: Compressing objects: 100% (8/8), done.
remote: Total 34 (delta 23), reused 26 (delta 22), pack-reused 3
Unpacking objects: 100% (34/34), done.
From <https://github.com/cloudposse/terraform-aws-rds>
 * [new branch]      0.11/master            -> upstream/0.11/master
...
 * [new tag]         0.28.0                 -> 0.28.0

 ➜ git merge upstream/master
Updating a802859..c682edb
Fast-forward
 ...
 ...
 create mode 100644 test/src/go.sum
Amit Karpe avatar
Amit Karpe
 ➜ git push
Total 0 (delta 0), reused 0 (delta 0)
To <https://github.com/amitkarpe/terraform-aws-rds>
   a802859..c682edb  master -> master
Amit Karpe avatar
Amit Karpe

After this should run :

git checkout examples/mssql-complete
git rebase master 
git checkout master
git rebase master examples/mssql-complete

Will this will rebase to master?

Joe Niland avatar
Joe Niland

No, the above commands should have been run from your feature branch

Joe Niland avatar
Joe Niland

So you can check it out and merge master into it and then push to update the PR

Amit Karpe avatar
Amit Karpe
 ➜ git checkout examples/mssql-complete
Already on 'examples/mssql-complete'
Your branch is behind 'origin/examples/mssql-complete' by 2 commits, and can be fast-forwarded.
  (use "git pull" to update your local branch)

 ➜ git merge upstream/master
Removing test/src/Gopkg.toml
Removing test/src/Gopkg.lock
Merge made by the 'recursive' strategy.
...
...

 ➜ gst
On branch examples/mssql-complete
Your branch and 'origin/examples/mssql-complete' have diverged,
and have 7 and 1 different commits each, respectively.
  (use "git pull" to merge the remote branch into yours)

nothing to commit, working tree clean

But push failed:

 ➜ gco master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.

 ➜ git push origin examples/mssql-complete
To <https://github.com/amitkarpe/terraform-aws-rds>
 ! [rejected]        examples/mssql-complete -> examples/mssql-complete (non-fast-forward)
error: failed to push some refs to '<https://github.com/amitkarpe/terraform-aws-rds>'
hint: Updates were rejected because a pushed branch tip is behind its remote
hint: counterpart. Check out this branch and integrate the remote changes
hint: (e.g. 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
Joe Niland avatar
Joe Niland

You need to git pull your feature branch

Amit Karpe avatar
Amit Karpe
➜ git pull
Already up to date!
Merge made by the 'recursive' strategy.

 ➜ gst
On branch examples/mssql-complete
Your branch is ahead of 'origin/examples/mssql-complete' by 8 commits.
  (use "git push" to publish your local commits)

nothing to commit, working tree clean

Pull was successful

 ➜ git log --oneline
4cbf0e3 (HEAD -> examples/mssql-complete) Merge branch 'examples/mssql-complete' of <https://github.com/amitkarpe/terraform-aws-rds> into examples/mssql-complete
564fe34 Merge remote-tracking branch 'upstream/master' into examples/mssql-complete
c682edb (tag: 0.28.1, upstream/master, origin/master, origin/HEAD, master) chore(deps): update terraform cloudposse/label/null to v0.22.1 (#84)
...
...

Should I push like this git push origin examples/mssql-complete?

Amit Karpe avatar
Amit Karpe

done

 ➜ git push origin examples/mssql-complete
Enumerating objects: 10, done.
Counting objects: 100% (10/10), done.
Delta compression using up to 8 threads
Compressing objects: 100% (4/4), done.
Writing objects: 100% (4/4), 637 bytes | 318.00 KiB/s, done.
Total 4 (delta 1), reused 0 (delta 0)
remote: Resolving deltas: 100% (1/1), completed with 1 local object.
To <https://github.com/amitkarpe/terraform-aws-rds>
   5e326ea..4cbf0e3  examples/mssql-complete -> examples/mssql-complete
Amit Karpe avatar
Amit Karpe
09:36:55 AM

I can see the latest commits at pull/79/commits Looks like “All checks have passed”

Joe Niland avatar
Joe Niland

Yes the pre checks are ok but need to check terratest. I just noticed one thing. Could you please change the region in fixtures to us-east-2?

Amit Karpe avatar
Amit Karpe

ok

Amit Karpe avatar
Amit Karpe
 ➜ ga .

 ➜ gcam "changed the region in fixtures to us-east-2"
[examples/mssql-complete f1e140b] changed the region in fixtures to us-east-2
 1 file changed, 46 insertions(+)
 create mode 100644 examples/mssql/fixtures.us-east-2.tfvars

 ➜ git push origin examples/mssql-complete
To <https://github.com/amitkarpe/terraform-aws-rds>
 ! [rejected]        examples/mssql-complete -> examples/mssql-complete (fetch first)
error: failed to push some refs to '<https://github.com/amitkarpe/terraform-aws-rds>'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first integrate the remote changes
hint: (e.g., 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

 ➜ gst
On branch examples/mssql-complete
Your branch is ahead of 'origin/examples/mssql-complete' by 2 commits.
  (use "git push" to publish your local commits)

nothing to commit, working tree clean

Solved:

 ➜ git pull
remote: Enumerating objects: 2, done.
remote: Counting objects: 100% (2/2), done.
remote: Total 2 (delta 1), reused 2 (delta 1), pack-reused 0
Unpacking objects: 100% (2/2), done.
From <https://github.com/amitkarpe/terraform-aws-rds>
   4cbf0e3..c95d1a9  examples/mssql-complete -> origin/examples/mssql-complete
Merge made by the 'recursive' strategy.
 README.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

 ➜ git push origin examples/mssql-complete
Enumerating objects: 17, done.
Counting objects: 100% (15/15), done.
Delta compression using up to 8 threads
Compressing objects: 100% (11/11), done.
Writing objects: 100% (11/11), 1.53 KiB | 520.00 KiB/s, done.
Total 11 (delta 5), reused 0 (delta 0)
remote: Resolving deltas: 100% (5/5), completed with 2 local objects.
To <https://github.com/amitkarpe/terraform-aws-rds>
   c95d1a9..88ee348  examples/mssql-complete -> examples/mssql-complete
Joe Niland avatar
Joe Niland

It seems the test may need a fix or maybe something in the module. Also your new test doesn’t seem to be called.

Amit Karpe avatar
Amit Karpe

Can I create new fork and try again?

Joe Niland avatar
Joe Niland

You can just keep pushing to your feature branch

Amit Karpe avatar
Amit Karpe

As of now I don’t have any plan to change any code. So there won’t be any new changes.

Joe Niland avatar
Joe Niland

If you can please ensure your test will be included in the terratest run

Amit Karpe avatar
Amit Karpe

I am sorry, but I have done any terratest for this module. How to do it? What is procedure to submit the test results?

Amit Karpe avatar
Amit Karpe

@Joe Niland test/terratest — tests failed

Amit Karpe avatar
Amit Karpe

My first open source contribution Thanks a million

1
Amit Karpe avatar
Amit Karpe

I must say that @Joe Niland guided me with tremendous patience Thanks again!!!

Joe Niland avatar
Joe Niland

No worries @

@Matt Gowie fixed the test issue

1

2020-12-07

2020-12-06

2020-12-03

Jurgen avatar
Jurgen
Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

bump me if they pass and we can merge

Jurgen avatar
Jurgen

all failing on readme..

Jurgen avatar
Jurgen

it seems you are on it. Love your work

1
Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

Thanks @

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

(btw, you can run make init && make readme to build it locally)

Jurgen avatar
Jurgen

kk, will do in the next round.. all of these modules are dependencies in other modules so once these are good and have new verions i’ll update the parents

Jurgen avatar
Jurgen

seems like the readme hates us

Jurgen avatar
Jurgen

bummer

Jurgen avatar
Jurgen
$ make init && make readme
Cloning <https://github.com/cloudposse/build-harness.git#master>...
Cloning into 'build-harness'...
remote: Enumerating objects: 129, done.
remote: Counting objects: 100% (129/129), done.
remote: Compressing objects: 100% (104/104), done.
remote: Total 129 (delta 5), reused 67 (delta 5), pack-reused 0
Receiving objects: 100% (129/129), 74.84 KiB | 210.00 KiB/s, done.
Resolving deltas: 100% (5/5), done.
Installing packages 0.133.0...
Cloning into '/Users/jurgen.weber/checkouts/deltatre/terraform-aws-route53-cluster-hostname/build-harness/vendor/packages'...
error: RPC failed; curl 18 transfer closed with outstanding read data remaining
fatal: error reading section header 'shallow-info'
make[1]: *** /Users/jurgen.weber/checkouts/deltatre/terraform-aws-route53-cluster-hostname/build-harness/vendor/packages/install: No such file or directory.  Stop.
make: *** [/Users/jurgen.weber/checkouts/deltatre/terraform-aws-route53-cluster-hostname/build-harness/modules/packages/Makefile:24: packages/install/gomplate] Error 2
Joe Niland avatar
Joe Niland

I think that’s a connectivity issue. Did you try a couple of times?

Jurgen avatar
Jurgen

yeah

Joe Niland avatar
Joe Niland

I haven’t seen it but I read it can be due to a slow connection somewhere

Joe Niland avatar
Joe Niland

You could try git fetch (https://stackoverflow.com/a/44151771/366965) in build-harness

error: RPC failed; curl transfer closed with outstanding read data remaining

I’m facing this error when I try to clone a repository from GitLab (GitLab 6.6.2 4ef8369): remote: Counting objects: 66352, done. remote: Compressing objects: 100% (10417/10417), done. error: RPC f…

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

I think an older version of git

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)
07:28:05 AM
Jurgen avatar
Jurgen

or anything I need to do to resolve?

Joe Niland avatar
Joe Niland

Do you still get that error?

Joe Niland avatar
Joe Niland

Is your git cli up to date?

Jurgen avatar
Jurgen

well, this is in your CI, yeah?

and yeah, its up to date.

Joe Niland avatar
Joe Niland

oh I thought the error was on your local machine

Jurgen avatar
Jurgen
$ git --version
git version 2.29.2
Joe Niland avatar
Joe Niland

i thought that was weird

Jurgen avatar
Jurgen

yeah, that error is from my machine but dunno

Jurgen avatar
Jurgen

seems to work today

Joe Niland avatar
Joe Niland

oh but actually we were talking about you running make init && make readme

Joe Niland avatar
Joe Niland

right

Jurgen avatar
Jurgen

all those PR’s are ticked now can we merge?

Joe Niland avatar
Joe Niland

if tests are passing, then yes

Joe Niland avatar
Joe Niland

i’ll take a look shortly

Joe Niland avatar
Joe Niland

thanks for the heads up

Jurgen avatar
Jurgen

cool, np

Jurgen avatar
Jurgen

these ones are all dependencies of something else, so i’ll do the parents next

Joe Niland avatar
Joe Niland

actually I left a note on one. the null-label module latest version is now 0.22.0 so I think it’d be good to reference that one

Jurgen avatar
Jurgen

boom, sure

Joe Niland avatar
Joe Niland

great!

Jurgen avatar
Jurgen

all done

Jurgen avatar
Jurgen

now its pushed

Joe Niland avatar
Joe Niland

Thanks

Joe Niland avatar
Joe Niland

3 done @

Jurgen avatar
Jurgen

yeah, I saw. Cool, bbs with the next round.

1
Jurgen avatar
Jurgen

next round there @Joe Hosteny @Erik Osterman (Cloud Posse)

Joe Niland avatar
Joe Niland

@ some changes to test-harness - I’m having a look. Do your branches allow maintainer edits?

Jurgen avatar
Jurgen

yeah, go for it

Joe Niland avatar
Joe Niland

Can you try:

make init
make terraform/rewrite-module-source

?

Jurgen avatar
Jurgen

on which one?

Joe Niland avatar
Joe Niland

any of those

Jurgen avatar
Jurgen
sed: 1: "s,"git::<https://github>. ...": \1 not defined in the RE
make: *** [/Users/jurgen.weber/checkouts/deltatre/terraform-aws-elasticache-redis/build-harness/modules/terraform/Makefile:49: terraform/rewrite-module-source] Error 1
Jurgen avatar
Jurgen

assumes gnu sed?

Joe Niland avatar
Joe Niland

yeah same for me

Joe Niland avatar
Joe Niland

on mac

Jurgen avatar
Jurgen

yeah

Joe Niland avatar
Joe Niland

I will try to get it working using the build-harness docker image

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@Jeremy (Cloud Posse)

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

@Joe Niland @ I am currently working on both test-harness and build-harness so you might want to take a break.

In general, tests are expected to be run in the [testing.cloudposse.co](http://testing.cloudposse.co) Docker image and the update scripts are expected to be run in the build-harness Docker image. We are using a new (to us) tool, terraform-config-inspect that is not installed in Geodesic or in build-harness at this time, though I am working on getting it into build-harness.

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

The bigger issue is that going forward, we are requiring that all providers and module sources use the registry format. Any changes to modules require that you also update those.

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

We are working on automating those changes so we can push them out everywhere, and quickly, but running into a few snags slowing us down.

Joe Niland avatar
Joe Niland

Hey @Jeremy (Cloud Posse) Thanks for the info.

I am trying to use the new make target to change the provider format so the tests will pass for Jurgen’s PRs.

I was trying to do this with the build-harness Dockerfile but just trying to figure out how to build it in the context of the Terraform module directory because it doesn’t seem to mount the module dir as a volume. There’s probably a way to do this I’m not aware of!

Joe Niland avatar
Joe Niland

So far I did this (from the root directory of the terraform-aws-iam-s3-user module:

make init
docker build . -t build-harness -f build-harness/Dockerfile
docker run build-harness terraform/rewrite-module-source 

it outputs the following but doesn’t change anything:

context.tf
main.tf
examples/complete/context.tf

It should modify a line such as this right? source = "git::<https://github.com/cloudposse/terraform-aws-iam-system-user.git?ref=tags/0.17.0>"

Joe Niland avatar
Joe Niland

ah it’ll be modifying it within the container

Joe Niland avatar
Joe Niland

if you can, please let me know how/if I can do the above!

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

Sorry, Joe, build-harness is not ready for this task yet. Still working on it. Give us another day or so.

Joe Niland avatar
Joe Niland

No worries, Jeremy. Appreciate all your work on this!

BTW, we might need a PSA on the #pr-reviews channel because there are quite a few PR’s waiting on the tests to pass.

2020-12-02

    keyboard_arrow_up