Beejones/updating npm packages #38
Conversation
tsconfig.json
Outdated
| @@ -11,6 +11,7 @@ | |||
| "sourceMap": false, | |||
| "resolveJsonModule": true, | |||
| "types": ["@types/node"], | |||
| "strict": false, | |||
There was a problem hiding this comment.
Why can't we use strict:true ?
There was a problem hiding this comment.
I am currently running the CI with strict: true. I did this experiment because of failing CI's in ACI. After this last run we can merge this version and we can remove the install of rollup from privacy-sandbox-dev
.github/devcontainer.json
Outdated
There was a problem hiding this comment.
What is the reason that we have two devcontainer.json?
There was a problem hiding this comment.
one for CI and one for the devcontainer. Are you saying we can have one for both?
There was a problem hiding this comment.
I see. I just didn't know about devcontainers/[email protected].
Are you saying we can have one for both?
I guess it's preferable, but what's your view on that? At least I don't think we need to address it within this PR.
| // committed_tcb | ||
| // committed_tcbx` |
There was a problem hiding this comment.
What is this change for?
There was a problem hiding this comment.
no idea how this happened. Will fix this
scripts/set_python_env.sh
Outdated
| pip install wheel | ||
| pip install -U -r ./requirements.txt |
There was a problem hiding this comment.
Why can't we install wheel in requirements.txt?
There was a problem hiding this comment.
good question. I used to only have it in requirements and it generated an error. This solves the error.
There was a problem hiding this comment.
Why does it solve the problem?
There was a problem hiding this comment.
Interesting. After fixing the versions of the packages, the error is gone and there is no more need to install wheel first. This must have been a conflict between installed packages
src/repositories/LastestItemStore.ts
Outdated
| @@ -45,7 +45,7 @@ export class LastestItemStore<K extends number, T> { | |||
| } | |||
|
|
|||
| public receipt(id: K) { | |||
| const version = this.store.getVersionOfPreviousWrite(id); | |||
| const version = this.store.getVersionOfPreviousWrite(id) || 0; | |||
There was a problem hiding this comment.
Is it really OK to set version=0?
I feel like we should return "not found" instead, but not sure
There was a problem hiding this comment.
Added a test for undefined
| let caller: ccfapp.AuthnIdentityCommon; | ||
| ): [ | ||
| ccfapp.AuthnIdentityCommon | undefined, | ||
| ServiceResult<string> | undefined, |
There was a problem hiding this comment.
Shouldn't we always return ServiceResult<string>? I think we want validator.validate(request) without ? after confirming validator is not null/undefined.
There was a problem hiding this comment.
I added a failed result so undefined is never returned.
| const policy = JwtValidationPolicyMap.read(issuer); | ||
| const policy = JwtValidationPolicyMap.read(issuer) || {}; |
There was a problem hiding this comment.
Shouldn't we return "issuer is not registered" error rather than using empty policy?
I guess I'm missing something, but I feel like there would be no check if issuer doesn't exist in JwtValidationPolicyMap?
|
Why do we need 2fe6bdf within this PR? |
I believed we needed it because I saw JWT failures in the container log. That's why I added it. We need this change anyway so lets keep it |
Update package.json packages. We want to fix the versions to we don't get surprises in case of updates.
Improve container image and streamline devcontainer and the ci dockerfile.
Fix pip install versions and fix the wheel error we we having
Merge beejones/cleanup3 into this PR which is about improving the code by leveraging strict typescript
Support arrays for JWT validation policies