[ColeJordan-mobilise]

Context

This PR adds the application and infrastructure changes needed to deploy and test Airbyte in a review app. Airbyte copies the Postgres database to BigQuery.

It is a draft while the Airbyte setup is tested with the infrastructure team. It is not ready to merge.

Changes proposed in this pull request

Terraform

• Adds the shared Airbyte module and the airbytehq/airbyte provider (pinned to 0.10.0).
• Reads these secrets from infrastructure_secrets: AIRBYTE-WORKSPACE-ID, AIRBYTE-CLIENT-ID, AIRBYTE-CLIENT-SECRET, AIRBYTE-REPLICATION-PASSWORD, AIRBYTE-BQ-SA. The infra team creates these by hand, and plan fails if they are missing while Airbyte is enabled.
• Turns Airbyte on for the review app only. pg_airbyte_enabled turns on the Postgres write-ahead logging that replication needs. It is already on in QA and production.
• Adds the BigQuery and Airbyte settings to the app config. The policy tag ID is built from shared locals, so it is not repeated.
• The Airbyte server URL is built in one local.

.NET (James's dfe-analytics bump)

• Updates DfeAnalytics.Core and DfeAnalytics.AspNetCore from 0.6.2 to 0.6.4.
• Adds DfeAnalytics.EFCore 0.6.4 to the Persistence project and sets DfeAnalyticsDbContext to PortalDbContext in Directory.Build.props.
• These are needed for the Airbyte rollout. The module runs in .NET mode (is_dotnet_application = true) and uses these to read our DbContext. Our own code does not use the package yet.
• This changes the analytics version that production runs.

These changes first came in PR #672. That branch had merge conflicts, so they were moved onto a fresh branch here.

Guidance to review

• Review app deploy
• Terraform plan and apply
• Airbyte setup and connection
• Build and automated tests

How it was checked

• The review app deploys and the Airbyte stream update job completes: the sync succeeds and BigQuery policy tags are updated.
• E2E tests on the review app pass (376 passed, 0 failed, 4 skipped).

Link to Trello card

https://trello.com/c/KrX6D2se/2997-enable-airbyte-in-service-cypmd

Checklist

• Attach to Trello card
• Rebased main
• Cleaned commit history
• Tested on the review app

[dfe-lance]

Review of PR #709 at commit 164dd3f: REQUEST CHANGES
PR: #709
Title: 2997-airbyte-review
Author: ColeJordan-mobilise
Base: main
Verdict: REQUEST CHANGES

Summary
The PR adds the Airbyte module, provider, config and secrets to the Terraform and turns it on for the review app. It also bumps the dfe-analytics packages to 0.6.4 and adds DfeAnalytics.EFCore to the Persistence project. The .NET changes aren't mentioned in the description and aren't explained. I could only read the src tree in this checkout. Access to terraform/ was denied, so I reviewed the Terraform from the diff alone and give no line numbers for it. I did not run any terraform plan.

Blocking issues
Unexplained .NET changes.

The description lists only airbyte.tf, variables/secrets and the provider.
The diff also does the following:
It bumps DfeAnalytics.Core and DfeAnalytics.AspNetCore from 0.6.2 to 0.6.4.
It adds a new DfeAnalytics.EFCore reference to src/DfE.CheckPerformanceData.Persistence/DfE.CheckPerformanceData.Persistence.csproj.
It sets DfeAnalyticsDbContext in src/Directory.Build.props.
DfeAnalytics.EFCore brings in System.CommandLine, Npgsql.EntityFrameworkCore.PostgreSQL and Microsoft.Extensions.Http, and changes the System.CodeDom version.
The Directory.Build.props value points at PortalDbContext. That type exists in src/DfE.CheckPerformanceData.Persistence/Contexts/PortalDbContext.cs:12, so the reference resolves.
Nothing in src/**/*.cs uses the EFCore package yet. I couldn't tell what it's for, or whether it is meant to be part of this change.
Please explain it in the description. If it isn't needed for the Airbyte deploy, split it into its own PR so this one stays infrastructure-only.
The project's rules say there should be no Directory.Build.props. This repo already has one, but adding a property there still needs a stated reason.
Commit history breaks the project's rules.

The recent branch commits are all titled Feat:airbyte config.
The rules ban feat:-style prefixes and ask for terse messages in the shape of recent history.
They also require a work-item reference on every commit: AB#nnnnnn, a (nnnnnn) scope, a leading nnnn - , or Refs #n.
The Trello link in the description doesn't satisfy that. The PR title uses 2997, which looks like a Trello card number rather than an AB item, so please confirm what the correct reference is.
The description says "Cleaned commit history", but three identical commits remain. Please squash and reword them.
Policy tag IDs are hardcoded a second time in terraform/application/application.tf.

This PR creates local.gcp_taxonomy_id and local.gcp_policy_tag_id in variables.tf, and dfe_analytics.tf and airbyte.tf use them.
BIGQUERY_POLICY_TAGS then repeats the same IDs as a literal string, along with the project name and region.
If the taxonomy changes, the module and the app config will disagree without any error.
Build the string from the locals, using jsonencode instead of the hand-escaped JSON. Move the project ID check-performance-data and the region into locals or variables as well.
Suggestions
airbyte_enabled has no type.
variable "airbyte_enabled" { default = false } has no type or description. connection_status right below it has both.
Add type = bool and a description.
Do the same for the new server_version in variables.tf, which needs type = string.
Redundant guards.
Inside module "airbyte", the module already has count = var.airbyte_enabled ? 1 : 0.
The var.airbyte_enabled ? ... : null guards on workspace_id, client_id and the other secrets are therefore unnecessary. Drop them to simplify the file.
The provider block in terraform.tf still needs its guards, because a provider is always configured.
Duplicated server URL.
https://airbyte-${var.namespace}.${module.cluster_data.ingress_domain} is built in three places: airbyte.tf, application.tf and terraform.tf.
Put it in one local and add the /api/public/v1 suffix only where it's needed.
Dead code.
local.gcp_dataset_name is declared in airbyte.tf but only used for BIGQUERY_AIRBYTE_DATASET. Check that the module derives the same dataset name. If it names the dataset itself, the two could drift.
Delete the commented-out #use_legacy_dotnet_shim line.
Delete the stray # Configuration options comment in the provider block.
Formatting.
The secret_variables merge is laid out oddly, with a leading comma on its own line.
There is a double blank line in airbyte.tf.
Run terraform fmt.
Secrets enabled only for review.
review.tfvars.json sets airbyte_enabled to true, and pg_airbyte_enabled is already true in qa.tfvars.json and production.tfvars.json.
The new module reads five secrets (AIRBYTE-WORKSPACE-ID, AIRBYTE-CLIENT-ID, AIRBYTE-CLIENT-SECRET, AIRBYTE-REPLICATION-PASSWORD, AIRBYTE-BQ-SA) from infrastructure_secrets. These must exist in the review Key Vault before apply.
I couldn't verify they exist, because Key Vault is a runtime service I can't reach from here. Please say in the description how they get populated.
Provider pinning. airbytehq/airbyte is pinned to the exact version 0.10.0, matching the other providers. That's fine. Make sure the .terraform.lock.hcl is updated if the repo commits one.
Description. The description's "Tested by running locally" can't cover a Terraform deploy of the review app. Say how it was verified, for example with a plan output or the review app run.
Tests
No automated tests are added. For Terraform and package-version changes that is understandable.
Because of the new DfeAnalytics.EFCore reference and the version bumps, the project's rules call for running unit, integration and E2E before calling it done.
The existing AnalyticsRegistrationTests and DfeAnalyticsServiceTests should be re-run against 0.6.4.

[github-actions]

Deployments

Review app is available at these URLs:
https://check-performance-data-pr-709.test.teacherservices.cloud

[dfe-lance]

Hi Jordan,

Thanks for the context, and no need to apologise. My comments came from reviewing the diff without the background you've given me now. A few were
badly worded, so sorry about that. The latest version is in much better shape and there's nothing blocking in it.

The PR to your own branch: that wasn't meant as a block. It got picked up because it was awaiting review. It's fine that you've deleted it.

Draft: could you set #709 to draft until it's ready? Then it won't get reviewed before you want it to. I'll take a proper look once it's ready.

Secrets / "only in review": that one was my wording, not your code. Enabling it only for review is right for this PR. I was only checking that the
five Key Vault secrets exist, and you've answered that: you populate them as part of the rollout, and the review app deploys. A line in the
description saying so would help the next reviewer.

pg_airbyte: not a question. I'd noted it's already on in QA and prod, so this PR only adds the Airbyte side. The write-ahead logging explanation makes
sense.

Tests: all fine now. The review app's E2E run passed (376 passed, 0 failed), and the deploy working answers my worry about the Terraform plan.

The .NET changes: that makes sense, thanks. Could you add a line to the description saying they're James's dfe-analytics bump to 0.6.4, needed for the
Airbyte rollout? My guess is the Airbyte module's dotnet mode (is_dotnet_application = true) needs DfeAnalytics.EFCore and the DfeAnalyticsDbContext
setting so it can read our DbContext. If that's right, say so. Nothing in our code uses the package yet, which is why it stood out. It also changes
the analytics version production runs, so a quick note on what changed between 0.6.2 and 0.6.4 would be good. If James can raise it as its own PR,
even better, but I won't hold this up for it.

When you take it out of draft, these are the things I'd still like:

1. Commits: squash them, and add the work item reference (AB#… or Refs #…) to the messages instead of "Feat:airbyte config". Is 2997 a Trello or Jira card? If so, what's the ADO ticket number? You may not have one if you don't use our ADO board.
2. Other environments: a terraform plan for one of them, where airbyte_enabled is false, showing no changes. I couldn't tell whether the null
   config-map values come out as missing keys or empty ones.
3. connection_status: worth starting it inactive on review and switching it on after checking the first sync? Your call.
4. BIGQUERY_POLICY_TAGS: build it from the taxonomy and policy-tag locals with jsonencode, rather than repeating the IDs, project and region.
5. Tidy-ups:
   • Add type/description to airbyte_enabled and server_version.
   • Build the Airbyte server URL in one local.
   • Drop the airbyte_enabled ? … : null checks inside the module block, since it already has count.
   • Remove the commented-out lines and the unrelated max_memory reformat, and run terraform fmt.

Shout if you want to go through any of it on a call.

[github-actions]

E2E tests failed

@ColeJordan-mobilise this is your pull request: please look at what failed and fix it, or say here why it is not the change.

375 passed, 1 failed, 4 skipped in 16 min 38 s

Commit e55658b against https://check-performance-data-pr-709.test.teacherservices.cloud (run)

Failed tests (1)

• Admin.DataEgressTests.The_runs_history_lists_a_transferred_run_with_its_count_and_a_draft_with_resume