Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CAPT-1632 Implement review apps #2737

Merged
merged 60 commits into from
Jun 7, 2024
Merged

Conversation

AbigailMcP
Copy link
Contributor

@AbigailMcP AbigailMcP commented May 9, 2024

Changes made in this PR

  • Ran the new_service command in the https://github.com/DFE-Digital/teacher-services-cloud repo and copied config files into this repo, updating where necessary
  • Updated Makefile so that we have existing commands still working plus new commands with -aks suffixes
  • Removed autogenerated development config and replaced with test env
  • Created new build_and_deploy workflow to deploy review app when 'deploy' label added
  • Created delete review app workflow on PR close

Guidance to review

  • Check that the review app is built correctly
  • Check that the review app and all related resources are destroyed when the PR is closed

Not included

  • We've left the old review app creation/deletion in the codebase to make the review simpler, will remove in follow up PR

Copy link

github-actions bot commented May 9, 2024

@AbigailMcP AbigailMcP force-pushed the capt-1632-implement-review-apps branch from 3b03f08 to 52be482 Compare May 13, 2024 14:11
@vacabor vacabor added the deploy Deploy a review app for this PR label May 14, 2024
@AbigailMcP AbigailMcP added deploy Deploy a review app for this PR and removed deploy Deploy a review app for this PR labels May 14, 2024
terraform/application/config/test.tfvars.json Outdated Show resolved Hide resolved
terraform/application/config/test.tfvars.json Outdated Show resolved Hide resolved
global_config/test.sh Outdated Show resolved Hide resolved
@vacabor vacabor requested review from slorek, saliceti and RMcVelia May 31, 2024 12:07
terraform/application/terraform.tf Outdated Show resolved Hide resolved
terraform/application/terraform.tf Outdated Show resolved Hide resolved
terraform/application/terraform.tf Outdated Show resolved Hide resolved
Copy link
Contributor

@slorek slorek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 Looking very good, only a handful of issues remaining.

These are the things I've tested:

✅ Basic auth
✅ DfE Identity sign in
✅ Claim submission
✅ Exception reporting to Rollbar
✅ Ordnance survey API address lookup
✅ HMRC API integration
✅ e-mails sent
✅ Admin login with DfE Sign In
✅ Levelling Up Premium Payments awards upload
✅ TPS data upload
✅ School data import
✅ Checked application and deployment logs for errors
✅ Opening a PR against another PR already deployed keeps the deployment status and PR comments separate from the base branch PR
✅ Close PR deletes the review app
✅ Reopening a close PR re-deploys the review app

Remaining Issues:

🛑 We should upgrade from Postgres 11 to Postgres 16 which we will be doing for the production environment at the same time as well
⚠️ dfe analytics lint check should be a required check in order to merge a PR. Currently the PR can be merged when the linter fails, which will result in a broken production deployment. We can set this as a required check once the PR is merged but we must remember to do so.
⚠️ If a migration is subsequently added to the branch after the initial deployment, the migration doesn't run. This is not a blocker as the migration can be run manually using kubectl but we would also have to restart the application pod as well. This can be addressed in a follow up ticket as the existing review apps implementation doesn't work either
⚠️ I'm unable to check the log output for migrations in GitHub as the migrations don't run and there's no output (despite it saying running migrations)
🛑 DQT client is not working (I've checked the credentials are present and correct; seems to be a networking issue but works locally):

irb(main):002:0> c = Dqt::Client.new
irb(main):005:0> c.teacher.find("1886092")
/usr/local/lib/ruby/3.2.0/net/http.rb:1603:in `initialize': Failed to open TCP connection to preprod.teacher-qualifications-api.education.gov.uk:443 (getaddrinfo: Try again) (Faraday::ConnectionFailed)

⚠️ Minor - deleting of a review app shows as a new deployment in GitHub rather than a modification of the existing deployment. Once deleted, it continues to show the deployment as active and 'successfully deployed'. We can fix this in a follow up ticket.
⚠️ We need to update the docs, removing out of date Heroku/Azure/Google account instructions and add some examples and document the namespace name etc.

AbigailMcP and others added 3 commits June 6, 2024 12:03
@vacabor vacabor merged commit 929357a into master Jun 7, 2024
17 checks passed
@vacabor vacabor deleted the capt-1632-implement-review-apps branch June 7, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy Deploy a review app for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants