Skip to content

[GPCAPIM-359] Sort out env vars#174

Merged
davidhamill1-nhs merged 75 commits intomainfrom
debt/GPCAPIM-359-sort-out-env-vars
Apr 27, 2026
Merged

[GPCAPIM-359] Sort out env vars#174
davidhamill1-nhs merged 75 commits intomainfrom
debt/GPCAPIM-359-sort-out-env-vars

Conversation

@davidhamill1-nhs
Copy link
Copy Markdown
Contributor

@davidhamill1-nhs davidhamill1-nhs commented Apr 16, 2026

Description

Context

Type of changes

  • Refactoring (non-breaking change)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would change existing functionality)
  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have followed the code style of the project
  • I have added tests to cover my changes
  • I have updated the documentation accordingly
  • This PR is a result of pair or mob programming
  • Exceptions/Exclusions to coding standards (e.g. #noqa or #NOSONAR) are included within this Pull Request.

Sensitive Information Declaration

To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.

  • I confirm that neither PII/PID nor sensitive data are included in this PR and the codebase changes.

Comment thread README.md Outdated
@ian-robinson-35
Copy link
Copy Markdown
Contributor

Generally looks great, a lot to see here. Just a couple of extra points:

  • On the new secrets directories could we add some readme file to explain what secrets are expected instead of the .gitkeep files. Otherwise I think the only way for know what to set would be to run and see what the error is. (If we don't know where to get the values from yet then just a list of expected secret files for each dir would be super useful)
  • Once the pipelines are passing again, it would be good to deliberately make a test fail and make sure that the PR checks fail too. Just because there's been some changes to how they run. It would be bad if we silently ignored any failures.

Comment thread gateway-api/src/gateway_api/pds/client.py Outdated
Comment on lines +131 to +136
log_details = {
"description": "PDS request",
"url": url,
"headers": headers,
}
logger.info(log_details)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I second this. I know we're not in any sensitive environments yet but this is the kind of thing we could accidentally leave in and cause problems. Better to filter out now and not have to come back to it (or just exclude header information for now if we just want to keep it simple)

Comment thread gateway-api/src/gateway_api/pds/test_client.py
Comment thread gateway-api/src/gateway_api/sds/test_client.py
Comment thread gateway-api/src/gateway_api/app.py Outdated
Comment thread gateway-api/tests/schema/test_openapi_schema.py
Comment thread gateway-api/tests/conftest.py Outdated
Comment thread scripts/env/app/provider.sh Outdated
env="$1"
case "$env" in
int|int-pds|int-sds)
echo "how_do_we_do_this?"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah this is a weird case isn't it. We're setup so that we can stub out the provider independently but the actual URL comes from SDS. We probably just need it to say "stub" or "not_stub". I think this is fine as-is for now and we can come back if / when we need to.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I changed it to stub, while you were writing this comment. I think it was one of copilot's review comments - it not being a url!

It seems to make sense to have it as stub, so we have control over it for now and get test patients in to the stub that will make a full request work with SDS INT systems and PDS INT patients.

LMK if you want it changing back, or to not-stub.

Comment thread scripts/env/env.mk Outdated
Comment thread Makefile Outdated
@sonarqubecloud
Copy link
Copy Markdown

@github-actions
Copy link
Copy Markdown

Deployment Complete

@davidhamill1-nhs davidhamill1-nhs merged commit a1b413d into main Apr 27, 2026
54 checks passed
@davidhamill1-nhs davidhamill1-nhs deleted the debt/GPCAPIM-359-sort-out-env-vars branch April 27, 2026 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants