-
Notifications
You must be signed in to change notification settings - Fork 89
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
chore: OpenAPI spec changes & other support for running localenv in docker windows #2530
Conversation
This reverts commit c4a0199.
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
# Conflicts: # package.json
✅ Deploy Preview for brilliant-pasca-3e80ec ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
RUN apk add --no-cache \ | ||
python3 \ | ||
make \ | ||
g++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was breaking on windows, and looks like not needed in general
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raducristianpopa specified in the original pr that this is for node-gyp
. #1795 (comment) Could node-gyp
be included with pnpm? It's liable to be used by some of our packages and its hard to say what. I did remove from backend and start localenv OK FWIW. Looks like npm does include an internal version (so maybe pnpm does too), although im not clear on if this would be available to packages: https://github.com/nodejs/node-gyp/blob/main/docs/Updating-npm-bundled-node-gyp.md#updating-the-npm-bundled-version-of-node-gyp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a dependency chain within testcontainers
that needed these packages (Python, C/C++ compiler) and I was following their requirements. I recall discussing with Max about this some time ago and it was working for me without installing them at that time.
@@ -0,0 +1,116 @@ | |||
openapi: 3.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no changes, just moved under openapi/specs
folder
noticed when trying to setup on windows that blaircurrey@blairs-MacBook-Pro-2 rafiki % pnpm -r build
Scope: 6 of 7 workspace projects
localenv/mock-account-servicing-entity build$ remix build
│ [info] building... (NODE_ENV=production)
│ [info] built (340ms)
└─ Done in 2.3s
packages/token-introspection build$ pnpm clean && tsc --build tsconfig.json && pnpm copy-files
│ > token-introspection@ clean /Users/blaircurrey/code/interledger/rafiki/packages/token-introspection
│ > rm -fr dist/
│ > token-introspection@ copy-files /Users/blaircurrey/code/interledger/rafiki/packages/token-introspection
│ > mkdir ./dist/openapi/specs && cp -r ./src/openapi/specs/*.yaml ./dist/openapi/specs/
└─ Done in 1.1s
packages/frontend build$ remix build
│ [info] building... (NODE_ENV=production)
│ [info] built (770ms)
└─ Done in 2.3s
packages/auth build$ pnpm build:deps && pnpm clean && tsc --build tsconfig.json && pnpm copy-files
│ > auth@ build:deps /Users/blaircurrey/code/interledger/rafiki/packages/auth
│ > pnpm --filter token-introspection build
│ > token-introspection@ build /Users/blaircurrey/code/interledger/rafiki/packages/token-introspection
│ > pnpm clean && tsc --build tsconfig.json && pnpm copy-files
│ > token-introspection@ clean /Users/blaircurrey/code/interledger/rafiki/packages/token-introspection
│ > rm -fr dist/
│ > token-introspection@ copy-files /Users/blaircurrey/code/interledger/rafiki/packages/token-introspection
│ > mkdir ./dist/openapi/specs && cp -r ./src/openapi/specs/*.yaml ./dist/openapi/specs/
│ mkdir: ./dist/openapi/specs: File exists
│ ELIFECYCLE Command failed with exit code 1.
│ /Users/blaircurrey/code/interledger/rafiki/packages/token-introspection:
│ ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL token-introspection@ build: `pnpm clean && tsc --build tsconfig.json && pnpm copy-files`
│ Exit status 1
│ ELIFECYCLE Command failed with exit code 1.
└─ Failed in 1.5s at /Users/blaircurrey/code/interledger/rafiki/packages/auth
packages/backend build$ pnpm build:deps && pnpm clean && tsc --build tsconfig.json && pnpm copy-files
│ > backend@ build:deps /Users/blaircurrey/code/interledger/rafiki/packages/backend
│ > pnpm --filter token-introspection build
│ > token-introspection@ build /Users/blaircurrey/code/interledger/rafiki/packages/token-introspection
│ > pnpm clean && tsc --build tsconfig.json && pnpm copy-files
│ > token-introspection@ clean /Users/blaircurrey/code/interledger/rafiki/packages/token-introspection
│ > rm -fr dist/
│ > token-introspection@ copy-files /Users/blaircurrey/code/interledger/rafiki/packages/token-introspection
│ > mkdir ./dist/openapi/specs && cp -r ./src/openapi/specs/*.yaml ./dist/openapi/specs/
└─ Running...
/Users/blaircurrey/code/interledger/rafiki/packages/auth:
ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL auth@ build: `pnpm build:deps && pnpm clean && tsc --build tsconfig.json && pnpm copy-files`
Exit status 1 See |
@BlairCurrey got it, thanks! Updated the |
# Conflicts: # openapi/auth-server.yaml # openapi/schemas.yaml # scripts/fetch-schemas.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there something in the docs that talks about when to run the postinstall?
|
Of course 🤦♀️ |
Changes proposed in this pull request
BEFORE_MERGED
variable from ourlocalenv:compose
commandsContext
This PR makes localenv work inside docker for windows (whether that's through hyper-v or WSL).
Symlinked Schemas
The main issue that prevented this working previously were the symlinked OpenAPI schemas (in
packages/*/src/openapi
) that docker for windows couldn't copy over as a proper file when creating the docker images.This led me down the road to a bigger refactor of how we were handling OpenAPI specs in the repo in general.
Usually, we used the
fetch-schemas
script to pull the latest OpenAPI schemas into the rootopenapi
folder, where we would from there, symlink to them frompackages/(backend | auth | token-introspection)/src/openapi
. We needed to do that for:auth-server.yaml
, for example, how we do intoken-introspection.yaml
:rafiki/packages/token-introspection/src/openapi/token-introspection.yaml
Lines 70 to 71 in 96061ff
The first "problem" was solved by being able to directly import the OpenAPI objects from the updated
open-payments
library:allowing us to do validation as such:
rafiki/packages/backend/src/index.ts
Lines 157 to 165 in aa55604
This is handy because it allows us to remove the
fetch-schemas
script, and it means that the spec in the open payments client we use in the repo will always be consistent with the spec that we validate against when checking the Open Payment API requests and responses.However, this didn't fully solve the issue 2: how can we still have references to the yaml files such that we could extend them in our other OpenAPI files (namely,
token-introspection.yaml
andid-provided.yaml
, which both reference theauth-server.yaml
& sharedschemas.yaml
?This was a bit tricky, and maybe there is a better solution, but this ends up working by using a
postinstall
command, that copies over the bundled yaml files in thenode_modules/@interledger/open-payments/...
folder into the respective(auth | token-introspection)/src/openapi/specs
folder when doingpnpm install
:rafiki/packages/token-introspection/package.json
Lines 13 to 15 in dceeb40
Since this ends up being kind of a generated file, we should
.gitignore
them:rafiki/.gitignore
Lines 60 to 64 in aa55604
Question
Related to a suggestion from @BlairCurrey: should we have a sibling
openapi
package in the repo that basically holds all of our OpenAPI yaml files, such that we can export and use the the OpenAPI validators inbackend
andauth
all fromone spot?
This way, we can just do this
postinstall
command in one place to get the Open Payments specs. The downsides of this: the files thatbackend
andauth
use separately are not in their corresponding repos and we would need to reference this package when generating the TS types for thetoken-introspection
package.Other issues
Other issues that prevented running localenv in windows docker were:
pnpm localenv:compose
commands do not work because of theBEFORE_MERGED
variable usageFixes #1562
Checklist
fixes #number