-
Notifications
You must be signed in to change notification settings - Fork 534
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: use nx commands #2493
chore: use nx commands #2493
Changes from 16 commits
22e58cd
d71f1cc
8fc8034
8c9ec9a
858fa77
e5c6bb7
97e7695
615401d
83781d0
95a4557
16122d5
f062d99
6acd051
b181935
6b5df30
6cec554
437637e
6aa6084
c7e6520
cef0847
7ab5103
59fb12c
3ad305c
ff866c0
530fdbb
07e998a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,34 @@ on: | |
pull_request: | ||
|
||
jobs: | ||
build: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note for reviewer: same here for unit tests. I guess a single compile step can be done for both workflows but I think this will add complexity to them. This way both workflows are independent and still we reduce the resource usage |
||
strategy: | ||
fail-fast: false | ||
runs-on: ubuntu-latest | ||
env: | ||
NPM_CONFIG_UNSAFE_PERM: true | ||
NODE_OPTIONS: --max-old-space-size=4096 | ||
steps: | ||
- name: Checkout | ||
uses: actions/checkout@v4 | ||
with: | ||
fetch-depth: 0 | ||
- uses: actions/setup-node@v4 | ||
with: | ||
node-version: 16 | ||
- name: Install | ||
run: npm ci | ||
- name: Build | ||
run: npm run compile | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a little scared of the "if
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We can do even another twist on this because:
Since the published packages are compiled using nodejs v16 maybe is better to have a "production" build as tests assets instead of a different build with each nodejs version (14, 18, 20, ...)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We don't have them anymore AFAICT. The last was removed with grpc-cencus-binary-propagator (#2276) because it always built on macOS (it had pre-built binaries for linux+glibc so building sometimes does not happen based on which system one is using). I think if there was another one left we'd have noticed it by now as it tends to take a lot of time on install. 🙂
I think having the Node.js version matched with the published packages makes sense from a testing standpoint. This way we can preempt surprises if there are any. @david-luna would you mind putting a comment in the publish workflow and here that the versions should always be updated together? 🙂 |
||
- name: Upload build artifact | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
name: tests-build-cache-${{ github.run_number }} | ||
path: node_modules/.cache/nx | ||
retention-days: 1 | ||
|
||
unit-test: | ||
needs: build | ||
strategy: | ||
fail-fast: false | ||
matrix: | ||
|
@@ -128,6 +155,11 @@ jobs: | |
run: npm install -g npm@9 # npm@9 supports node >=14.17.0 | ||
- name: Install | ||
run: npm ci | ||
- name: Download Build Artifacts | ||
uses: actions/download-artifact@v4 | ||
with: | ||
name: tests-build-cache-${{ github.run_number }} | ||
path: node_modules/.cache/nx | ||
- name: Build | ||
run: npm run compile | ||
- name: Unit tests (Full) | ||
|
@@ -145,6 +177,7 @@ jobs: | |
verbose: true | ||
|
||
browser-test: | ||
needs: build | ||
strategy: | ||
fail-fast: false | ||
matrix: | ||
|
@@ -166,6 +199,11 @@ jobs: | |
run: npm install -g npm@9 # npm@9 supports node >=14.17.0 | ||
- name: Install | ||
run: npm ci | ||
- name: Dowload Artifacts | ||
david-luna marked this conversation as resolved.
Show resolved
Hide resolved
|
||
uses: actions/download-artifact@v4 | ||
with: | ||
name: tests-build-cache-${{ github.run_number }} | ||
path: node_modules/.cache/nx | ||
- name: Build | ||
run: npm run compile | ||
- name: Unit tests | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,6 @@ | |
"compile": "tsc -p .", | ||
"lint": "eslint . --ext .ts", | ||
"lint:fix": "eslint . --ext .ts --fix", | ||
"precompile": "tsc --version && lerna run version:update --scope @opentelemetry/resource-detector-alibaba-cloud --include-dependencies", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note for reviewer: not necessary since |
||
"prewatch": "npm run precompile", | ||
"prepublishOnly": "npm run compile", | ||
"test": "nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts'", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
{ | ||
"tasksRunnerOptions": { | ||
"default": { | ||
"runner": "nx/tasks-runners/default", | ||
"options": { | ||
"cacheableOperations": [ | ||
"compile", | ||
"lint", | ||
"version:update" | ||
] | ||
} | ||
} | ||
}, | ||
"targetDefaults": { | ||
"compile": { | ||
"dependsOn": [ | ||
"version:update", | ||
"^compile" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note for reviewer: this is the syntax to tell that dependencies within the monorepo should be compiled before compiling the current package |
||
], | ||
"inputs": [ | ||
"{projectRoot}/src", | ||
"{projectRoot}/test" | ||
], | ||
"outputs": [ | ||
"{projectRoot}/build" | ||
] | ||
}, | ||
"lint": { | ||
"inputs": [ | ||
"{projectRoot}/src", | ||
"{projectRoot}/test" | ||
] | ||
}, | ||
"version:update": { | ||
"inputs": [ | ||
"{projectRoot}/package.json" | ||
], | ||
"outputs": [ | ||
"{projectRoot}/src/version.ts" | ||
] | ||
} | ||
} | ||
} | ||
david-luna marked this conversation as resolved.
Show resolved
Hide resolved
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
note for reviewer: this is the build step that is going to be cached.
github.run_number
is unique for the PR so each time the workflow is run this tasks overwrites the artifact saving space.retention-days
is set to 1 so the artifact will be removed the next day after the last run of this workflow.