#pr-reviews (2020-12)

ok, I’ve kicked off /test all everywhere

bump me if they pass and we can merge

all failing on readme..

it seems you are on it. Love your work

Thanks @Jurgen

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

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

seems like the readme hates us

bummer

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

yeah

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

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

I think an older version of git

or anything I need to do to resolve?

Do you still get that error?

Is your git cli up to date?

well, this is in your CI, yeah?

and yeah, its up to date.

oh I thought the error was on your local machine

i thought that was weird

yeah, that error is from my machine but dunno

seems to work today

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

right

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

if tests are passing, then yes

i’ll take a look shortly

thanks for the heads up

cool, np

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

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

boom, sure

great!

all done

now its pushed

Thanks

3 done @Jurgen

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

https://github.com/cloudposse/terraform-aws-iam-s3-user/pull/26 https://github.com/cloudposse/terraform-aws-dynamodb/pull/71 https://github.com/cloudposse/terraform-aws-elasticsearch/pull/84 https://github.com/cloudposse/terraform-aws-elasticache-redis/pull/92 https://github.com/cloudposse/terraform-aws-efs/pull/62

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

all of tem seem to be dying here? https://github.com/cloudposse/actions/runs/1510466275?check_suite_focus=true

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

yeah, go for it

Can you try:

make init
make terraform/rewrite-module-source

?

on which one?

any of those

assumes gnu sed?

yeah same for me

on mac

yeah

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

@Jeremy G (Cloud Posse)

@Joe Niland @Jurgen 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.

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.

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.

Hey @Jeremy G (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!

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>"

ah it’ll be modifying it within the container

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

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

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.

last one I think; https://github.com/cloudposse/terraform-aws-s3-bucket/pull/65

Hey @Mikhail Naletov — 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.

Nice improvement!

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…

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

http://github.com/mergify

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

@Jeremy G (Cloud Posse)

yes, mergify is the bot account for the mergify app

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

you guys sound busy.

how is all of this going?

very close! @Jeremy G (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.

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

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

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

I already commented on it earlier

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?

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

Sure go ahead and submit it

Can someone review the pr? Thanks

@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?

@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?

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.

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

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.

Any suggestion?

You need to do

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

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

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?

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

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

 ➜ 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.

You need to git pull your feature branch

➜ 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?

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

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

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?

ok

 ➜ 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

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

Can I create new fork and try again?

You can just keep pushing to your feature branch

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

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

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

@Joe Niland test/terratest — tests failed

My first open source contribution Thanks a million

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

No worries @Amit Karpe

@Matt Gowie fixed the test issue

please look at your fork permissions

it seems that is restricting the merge

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?

nevermind, I think this is in our side

Merged and released as 0.38.0 — Thanks @mihai.plesa!

perfect timing, thank you

related to: https://sweetops.slack.com/archives/CB6GHNLG0/p1607949992230100

@Maxim Mironenko (Cloud Posse)

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

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

Done

Thanks! @Joe Niland

Np

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”.

So mergify is still very relevant.

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

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

Fixed

Thank you!

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

sure! safe travel

@Maxim Mironenko (Cloud Posse)

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.