#pr-reviews (2024-10)

@Igor Rodionov @Jeremy G (Cloud Posse)

@Igor Rodionov @Ben Smith (Cloud Posse)

I don’t believe it’s too late

Merged thanks to Igor! Appreciate it

@Andriy Knysh (Cloud Posse)

@Joe Niland @johncblandii

a couple tests failed. I’m not sure if it is related to this PR, though.

https://github.com/cloudposse/terraform-aws-dms/actions/runs/11371070586/job/31632321574

@Igor Rodionov any ideas why terratests are failing for this PR?

@Ray Finch @johncblandii I fixed tests concurrency problem. now tests are runnung in seq

but there is still error

could you handle it ?

i believe i saw that before. the issue was specific to not setting an engine version since they changed the API after 3.4. I don’t recall the specific fix, but I could check tomorrow

hey @Igor Rodionov @johncblandii I updated the engine_version to 3.5 and did another push

Any objection to adding a default to the engine_version variable?

PR seems OK now. I think we can leave the engine_version variable as is, but I will defer to others with more knowledge of DMS

So it’s all looking good. Time to merge?

Seems fine - have merged it. If there are any issues we can cut a new version.

Thanks for your contribution and patience @Ray Finch!

Hey all, I found a bug which I submitted a new PR for https://github.com/cloudposse/terraform-aws-dms/pull/38

I believe we made a conscious decision to not allow other organizations due to the potential supply chain attack vectors. We use this in all of our repos and the risk is too large.

Cc @Jeremy G (Cloud Posse)

@Erik Osterman (Cloud Posse) Does that mean the concept of build-harness-extensions is dead then?

i.e. you don’t expect other organisations to reuse build-harness

If so, this part of the readme needs an update because you will now always need to fork build-harness if you want readme functionality, and why wouldn’t you want a readme. https://github.com/cloudposse/build-harness?tab=readme-ov-file#extending-build-harness-with-targets-from-another-repo

Let me discuss this with @Jeremy G (Cloud Posse) and see what we can do

I didn’t actually discuss with @Erik Osterman (Cloud Posse) yet, but his comment above was correct and I have closed/rejected the PR. It is a significant security risk that I do not want to detail in public.

If you want to pull a README template from some place other than Cloud Posse’s repos, you can fetch one externally in advance however you want and then set README_TEMPLATE_FILE to point to it.

Ok that implies the way we did it before with the template in the build-harness-extensions repo and setting that variable should still be working so I’ll have to dig into what’s breaking there. I’d assumed that variable didn’t work anymore since the template fetching code appeared and our setup broke at the same time.

Previously, the template file was cloned together with the repo. When we switched to GHE and introduced multiple github orgs, we needed a way to vary the template by org.

If it doesn’t find the template, it tries to download it from the organizations .github configuration repo

What @Jeremy G (Cloud Posse) says should work. Just modify your Makefile to specify the path to a pre existing template, and it should continue to work.

If you export README_TEMPLATE_FILE=/path/to/file.tmpl before running make readme it should use that template. If that is not working, then I would welcome a PR to fix it, or might fix it myself if you can explain what is broken.

I’ll have another go on Monday and see if I can pin down the issue. Thanks for the detail.

If I had to guess, it would be that you set README_TEMPLATE_FILE to a relative path that is relative to the wrong directory.

There were a few errors: • We use BUILD_HARNESS_EXTENSIONS_PATH in the README_TEMPLATE_FILE definition but were using it before it had been assigned • github_repo doesn’t exist in our README.yamls because we use gitlab_repo instead, which causes dirname to output errors when assigning the variables for fetching remote templates • README_INCLUDES stopped existing so that caused a crash when doing the template These did all have simple enough fixes in the end. • Fixed the order of variable assignments. • Setting README_TEMPLATE_REPO_ORG in our bootstrap Makefile means that it doesn’t go looking for github_repo , and as the README_TEMPLATE_FILE exists the org never gets looked at anyway so it can really be set to anything. • Switching to using env.GetEnv in the template fixed the missing README_INCLUDES issue. So in the end it did all work, it just needed some digging in to and I was distracted by the changes that made it seem like our use case wasn’t supported anymore when in fact it was, so thank you for the clarification that it was meant to be working as otherwise I’d probably have given up.

I have put a comment with similar content on the issue I had opened and closed that issue. https://github.com/cloudposse/build-harness/issues/387

Thanks @irl for the update! Glad it all worked out.

Yes, thank you @irl for the detailed set of fixes. I’m sure that will help others in the future.

@Dan Miller (Cloud Posse)

@Jeremy G (Cloud Posse) @Dan Miller (Cloud Posse) @Andriy Knysh (Cloud Posse)

@Markus Why are you using regional dns-delegated components? DNS is not regional in any way, and our architecture is set up for you to use a single dns-delegated component per account.

Your dns-delegated component actually describes, that it can be deployed regionally.

Not all components are set up to do so, e.g. aurora-postgres which has no way to point to a regional endpoint. We need postgres-writer.euw1.our-infrastructure.cloud and postgres-writer.use1.our-infrastructure.cloud which are entirely separate environments. The component only provisions postgres-writer.our-infrastructure.cloud when provisioned with a global dns-delegated zone.

This is a change that is non-intrusive and has been made across components (e.g. aurora-postgres), which is much easier to do than to alter components, into setting custom DNS names.

@Markus I don’t know why the dns-delegated component says it can be deployed regionally. Of course, the gbl region is not a real region, so it has to be deployed regionally in the sense that you have to use some regional endpoint to deploy it, but it has global effect. I will check but I think we should remove the guidance that it can be deployed regionally.

I think it is always a mistake to deploy it regionally, because it can cause the components to fight over what is actually the same resource.

In your case, you can set the regional designator in var.cluster_dns_name_part and var.reader_dns_name_part. Set var.cluster_dns_name_part to writer.euw1 or writer.use1 as appropriate (and likewise for reader), and continue to use a single global dns-delegated component.

Edit: just saw the PR comments, thanks for clarifying. Appreciate all of your work and efforts to open-source these components.

Unfortunately, without access to the reference architecture (no budget for it), there’s a lot of guesswork on my part, how things should be and sometimes I misinterpret.

@Markus did you know that the docs are now entirely public? https://docs.cloudposse.com/

Yes the jumpstart package / repo configuration itself is still private, but the rest is public! You should be able to make out quite a bit from here

Yes, worked through a lot of this in the last few weeks. You did an awesome job on this! Combined with the docs repo and the SweetOps Archive, you can piece together a lot of the reference architecture (e.g. from all the workflows).

I’ve been following Cloudposse since before the first office hours, so a lot of it are historic assumptions that might have been wrong from the start, especially because I had a few gaps due to me not working on infrastructure in my job.

ah okay great! thanks for the support! We’ve also been using GitHub discussions more frequently lately, so if you see any issues or have any requests for docs, this would be a great place to create a topic: https://github.com/orgs/cloudposse/discussions

@Bob Berg Thank you for this PR.

At the moment, the automated tests are failing. The test failures do not appear to be related to this PR, but still, we do not like to accept/merge PRs when the tests are failing. I am looking into it; no further action is required on your part at this time. However, because the failure involves Datadog rejecting what was previously a valid test, it will take quite a while to figure out.

@Bob Berg I was able to resolve the remaining issues. Your PR (with additions) has been approved, merged, and released in module version 1.4.2

Thank you again for your contribution and patience.

CC @Gabriela Campana (Cloud Posse)

Thanks a bunch! I was able to successfully use the new release to provision Synthetics with Advanced Scheduling.

Note see the comment here

https://sweetops.slack.com/archives/CUJPCP1K6/p1729818458931139?thread_ts=1729703106.614279&channel=CUJPCP1K6&message_ts=1729818458.931139

valkey seems to be a cheaper and fully open source alternative to redis

@Jeremy G (Cloud Posse)

Changes requested

Thanks for the initial review.

I addressed the feedback, please re-review.

Approved and merged

Thank you!!

@Jeremy G (Cloud Posse)

Small change requested

I addressed the issue.

Thanks for catching it!

Approved

Thanks!

@Jeremy G (Cloud Posse)

thank you Gabriela

@Jeremy G (Cloud Posse) is out for the rest of the week. Let’s have him take a look next week @Gabriela Campana (Cloud Posse)

just rebased/regenerated docs to resolve the conflict

@mihai.plesa PR approved, merged, released as v1.12.0

excellent, we upgraded. thank you

@johncblandii since you are working on this right now, can you review

Cc @Gabriela Campana (Cloud Posse)

@johncblandii bumping this up

@johncblandii approved it

@Gabriela Campana (Cloud Posse) a few tests failed, do I need to do anything to resolve?

@Igor Rodionov