LIME-1864 BE skeleton#1
Conversation
JessWinterborne
left a comment
JessWinterborne
left a comment
There was a problem hiding this comment.
Looking really good :)
|
One more thing - I agree with not adding all the GHAs since thats for another future ticket - but having check PR could be worth including here. |
SarahHillGDS
force-pushed
the
LIME-1864
branch
6 times, most recently
from
6dd6d67 to
3a9fbdf
Compare
JessWinterborne
left a comment
JessWinterborne
left a comment
There was a problem hiding this comment.
LGTM - might be worth getting another tick though
| path: coverage/ | ||
| - name: Run SonarCloud Analysis | ||
| if: ${{ github.actor != 'dependabot[bot]' }} | ||
| uses: SonarSource/sonarcloud-github-action@master |
There was a problem hiding this comment.
Could be picked up when we do github actions but there is a shared action for sonar we could reuse
There was a problem hiding this comment.
I've explicitly added this to the scope of the actions ticket https://govukverify.atlassian.net/browse/LIME-2030
| version: "0.1" | ||
| title: "Open Banking Credential Issuer Private API" | ||
| paths: | ||
| /basic-function: |
There was a problem hiding this comment.
Can we add things we know will exist session and authorization
There was a problem hiding this comment.
@DavidIndiongcoGDS has added this as part of LIME-1867
| version: "0.1" | ||
| title: "Open Banking Credential Issuer Public API" | ||
| paths: | ||
| /health: |
There was a problem hiding this comment.
Can we add things we know will exist token credential issue and jwks
|
|
||
| Globals: | ||
| Function: | ||
| CodeSigningConfigArn: !If |
There was a problem hiding this comment.
I think were missing the global timeout here for lambdas
There was a problem hiding this comment.
Setting to 40 seconds until we know more about response times
| - ProvisionedConcurrentExecutions: !FindInMap [ ProvisionedConcurrency, Environment, !Ref 'Environment' ] | ||
| - !Ref AWS::NoValue | ||
| Metadata: | ||
| BuildMethod: makefile |
There was a problem hiding this comment.
Should this be esbuild rather than makefile?
There was a problem hiding this comment.
vite is doing the building so the makefile was a convenient way to handle that from the template
we could remove vite and rely only on esbuild but as that's what the FE is using we aligned them, given we've got a working stable vite build config it's probably not worth the additional churn to remove it
| PublicAPIUsagePlan: | ||
| Type: AWS::ApiGateway::UsagePlan | ||
| Condition: IsDeployedFromPipeline | ||
| # DependsOn: |
There was a problem hiding this comment.
These are needed otherwise assets get created out of order but stage is created alongside the api
| # # | ||
| # Parameters # | ||
| # # | ||
| #################################################################### |
There was a problem hiding this comment.
Under parameters one we know we will need will be VerifiableCredentialKmsSigningKeyParameter
| @@ -0,0 +1,15 @@ | |||
| import type { APIGatewayProxyEvent, Context } from 'aws-lambda' | |||
There was a problem hiding this comment.
Do we have a basic infra test? Should be able to lift this from the work @ChrisBates1 has done
There was a problem hiding this comment.
Raised this with Chris last sprint, his preference is to handle this in a separate ticket
| @@ -0,0 +1 @@ | |||
| {} | |||
There was a problem hiding this comment.
Is there nothing that could be populated here based on what we have?
| minify: false, | ||
| outDir: 'dist', | ||
| rollupOptions: { | ||
| external: [/^@aws-sdk\/.*/, /^node:.*/, /^@aws-lambda-powertools\/.*/], //dependencies that should not be bundled |
There was a problem hiding this comment.
Can we add a little detail on why these shouldnt be bundled. Best assuming people coming after us may not know as much about vite. Or we might need to change or add to this in future so context is important
There was a problem hiding this comment.
they are excluded because they're already available in the lambda runtime
node also throws up some issues as some of it is installed OS specific
| @@ -0,0 +1,5 @@ | |||
| build-BasicFunction: | |||
There was a problem hiding this comment.
when we add more functions we could dry this up a bit like so:
.PHONY: bundle
bundle:
npm ci
npm run build
build-BasicFunction: bundle
cp -r dist/* $(ARTIFACTS_DIR)/
build-AnotherFunction: bundle
cp -r dist/* $(ARTIFACTS_DIR)/we're still running npm ci for each function build though so we might want to look into externalising that bit somehow
SarahHillGDS
dismissed stale reviews from DavidIndiongcoGDS and JessWinterborne
via
fbbae1a
nlsteers
force-pushed
the
LIME-1864
branch
12 times, most recently
from
6b1d474 to
5888e25
Compare
- add example metrics - add example logging - remove vite config - remove makefile - enable experimental decorators in tsconfig - update deploy template to use `esbuild` build method - align GHA with FE
|
5 participants
Proposed changes
What changed
quality-gate.manifest.json
acceptance-tests/Dockerfile
Why did it change
Establishing foundational structure for Open Banking API
Issue tracking
Other considerations
/common-cri-parameters/CriIdentifier
/common-cri-parameters/AuditEventNamePrefix