ci(commerce): Build @daffodil/commerce schematics#4342
ci(commerce): Build @daffodil/commerce schematics#4342joannalauu wants to merge 22 commits intodevelopfrom
@daffodil/commerce schematics#4342Conversation
joannalauu
force-pushed
the
ci/commerce-test
branch
from
5863788 to
6a190d5
Compare
joannalauu
changed the title
@daffodil/commerce schematics@daffodil/commerce schematics
joannalauu
force-pushed
the
ci/commerce-test
branch
from
1ec790b to
420b752
Compare
| # Driver: demo, standalone, app.config.ts | ||
| desc="driver=demo (standalone, app.config.ts)" | ||
| copy_base_app "$STANDALONE_APP_DIR" "$WORK_DIR/case-1" | ||
| raise_budget | ||
| if run_ng_add "demo" && npx ng build 2>&1; then | ||
| report_result "$case_num" "$desc" "pass" | ||
| else | ||
| report_result "$case_num" "$desc" "fail" | ||
| fi |
There was a problem hiding this comment.
Each case data should be in some kind of serializable format, e.g. JSON, so that cases can easily be added and changed without modifying this file.
This will require some abstraction around what you're doing in case 7. Maybe the JSON can reference some override files that this script will know how to use. Or we have a few hardcoded starting points that the script can use.
Hopefully that makes sense. The goal is that this just becomes a for loop and the switch case is removed.
joannalauu
force-pushed
the
ci/commerce-test
branch
8 times, most recently
from
03a91bc to
5e9f478
Compare
| node-version: 22.21.x | ||
| use-stamp-cache: true | ||
|
|
||
| - name: Start Verdaccio |
There was a problem hiding this comment.
I think this can be replaced with a container running in the pipeline: https://www.verdaccio.org/docs/docker/
There was a problem hiding this comment.
I looked into this approach, and I believe that if we replace the existing composite action with service containers, we'll need to restart the container within setup job and test job. This is because the service starts before workflow steps, but the VERDACCIO_CONFIG repository file and the Verdaccio cache is only available during the workflow steps. These restarts seem wasteful - do you think it is still worth it to go forward with this change, or is there an existing approach that can prevent these restarts while using service containers?
There was a problem hiding this comment.
I don't think the Verdaccio cache is particularly valuable, this is essentially the same as our build cache which should already be available to this job.
| sed -i "s/version = '.*'/version = '${{ env.TEST_VERSION }}'/" \ | ||
| tools/schematics/ng-add/generators/version.ts | ||
|
|
||
| - name: Build @daffodil/* packages |
There was a problem hiding this comment.
We need some way to describe these as dependencies of @daffodil/commerce (perhaps devDependencies) so that we don't name them explicitly here.
|
|
||
| - name: Publish @daffodil/* packages to Verdaccio | ||
| run: | | ||
| for pkg in ${{ env.DAFFODIL_PACKAGES }}; do |
There was a problem hiding this comment.
similarly to the build task, I think this should just be a npx nx run @daffodil/commerce:publish
|
|
||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 22.21.x |
There was a problem hiding this comment.
I'm not sure if moving node-version to matrix is worth it, because we will have to generate additional base apps with the different node-versions, but base apps are generated in the setup step, before the test matrix is defined. Testing node version support may add more complexity than its worth, but let me know if you'd still like node version to be tested, and I'll think of a structure to support this
| with: | ||
| path: | | ||
| ${{ env.WORK_DIR }} | ||
| ${{ env.VERDACCIO_STORAGE }} |
There was a problem hiding this comment.
I don't think we should cache verdaccio. build / publish in the job should be fast.
There was a problem hiding this comment.
build/publish takes approximately a minute total, so I currently continue caching verdaccio, but let me know if you still want me to move this to the test job
| name: Build | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
use latest action version.
| with: | ||
| fetch-depth: 1 | ||
|
|
||
| - uses: actions/setup-node@v4 |
| rm -rf "$TEST_DIR" | ||
| cp -a "$BASE_APP" "$TEST_DIR" | ||
|
|
||
| - name: Apply app modifications |
There was a problem hiding this comment.
This should be done with --standalone=false during generation.
| rm -f "src/app/app.routes.ts" | ||
| fi | ||
|
|
||
| - name: Generate schematic |
There was a problem hiding this comment.
To guarantee that users can do the exact command as we do in CI can you just do npx ng add @daffodil/commerce@$TEST_VERSION?
| EXTRA_FLAGS="--skip-package-json" | ||
| fi | ||
|
|
||
| if [ "${{ matrix.succeed }}" = "true" ] || [ "${{ matrix.build }}" = "true" ]; then |
There was a problem hiding this comment.
Instead of complex bash conditionals, I would just do continue-on-error: ${{ matrix.succeed }} for the tests we know are currently problematic.
There was a problem hiding this comment.
I am not sure if this is the best approach, because continue-on-error will allow the test to pass when we expect the step to fail but the step passes - this would prevent us from catching unexpected behavior and regressions. I implemented a fix that removes duplicate code - let me know if that would be sufficient
joannalauu
force-pushed
the
ci/commerce-test
branch
3 times, most recently
from
9b1244b to
092215e
Compare
joannalauu
force-pushed
the
ci/commerce-test
branch
from
06cab6b to
3cd2462
Compare
joannalauu
force-pushed
the
ci/commerce-test
branch
from
b78e74a to
3ad99ef
Compare
3 participants
PR Checklist
PR Type
Current behavior
Fixes: #4253
New behavior
Builds the following commerce schematic configurations:
Breaking change?
Additional context