CI: clarify the environment variables #1127

vmarta · 2023-04-12T19:48:18Z

Follow up to #1126, this PR clarifies the usage of some environment variables in CI:

renamed some variable for clarity
added some comments

And also fixed the other `IS_DEFAULT_NPM` (the one for pwa-kit-windows). Related to PR #1126

vmarta · 2023-04-12T21:21:50Z

.github/workflows/test.yml

        uses: "./.github/actions/create_mrt"
        with:
          mobify_user: ${{ secrets.MOBIFY_CLIENT_USER }}
          mobify_api_key: ${{ secrets.MOBIFY_CLIENT_API_KEY }}

      - name: Push Bundle to MRT (Development)
-        if: env.IS_NOT_FORK == 'true' && env.IS_LATEST_NPM == 'true' && env.DEVELOP == 'true'
+        if: env.IS_NOT_FORK == 'true' && env.IS_MRT_NODE == 'true' && env.DEVELOP == 'true'


Replacing with IS_MRT_NODE makes more sense because in this case:

first of all, what matters here is the Node version (not npm)

we're pushing a bundle to MRT, and MRT supports Node 16

while PWA Kit supports Node 16 and also 18

IS_LATEST_NPM originally made sense because the latest NPM used in PWA Kit (and CI too) at the time was for Node 16. But now it is no longer true because we're supporting Node 18 too.

vmarta · 2023-04-12T21:25:14Z

.github/workflows/test.yml

@@ -110,7 +113,7 @@ jobs:
        uses: "./.github/actions/check_clean"

      - name: Publish to NPM
-        if: env.IS_NOT_FORK == 'true' && env.IS_LATEST_NPM == 'true' && env.RELEASE == 'true'
+        if: env.IS_NOT_FORK == 'true' && env.IS_MRT_NODE == 'true' && env.RELEASE == 'true'


Does the change still make sense here? All of the previous changes above are for pushing a bundle to MRT. But here it's a difference scenario: publishing to NPM.

If you wanted to be extra generic, we could call it IS_PRIMARY_NODE and have a comment like "this is the primary version of node/npm we use for development - must be a version supported by MRT." That would indicate the broader use case, while still highlighting the important constraint.

I decided to leave it as is. I prefer the more concrete name.

vmarta · 2023-04-12T21:26:16Z

.github/workflows/test.yml

@@ -135,7 +138,11 @@ jobs:
    env:
      # The "default" npm is the one that ships with a given version of node.
      # For more: https://nodejs.org/en/download/releases/
-      IS_DEFAULT_NPM: ${{ matrix.node == 16 && matrix.npm == 8 || matrix.node == 18 && matrix.npm == 9 }}
+      # (We also use this env var for making sure a step runs once for the current node version)
+      IS_DEFAULT_NPM: ${{ (matrix.node == 16 && matrix.npm == 8) || (matrix.node == 18 && matrix.npm == 9) }}


Fixing the conditional logic here. The same fix that was on line 44 above.

wjhsf · 2023-04-13T15:01:09Z

.github/workflows/test.yml

@@ -110,7 +113,7 @@ jobs:
        uses: "./.github/actions/check_clean"

      - name: Publish to NPM
-        if: env.IS_NOT_FORK == 'true' && env.IS_LATEST_NPM == 'true' && env.RELEASE == 'true'
+        if: env.IS_NOT_FORK == 'true' && env.IS_MRT_NODE == 'true' && env.RELEASE == 'true'


If you wanted to be extra generic, we could call it IS_PRIMARY_NODE and have a comment like "this is the primary version of node/npm we use for development - must be a version supported by MRT." That would indicate the broader use case, while still highlighting the important constraint.

* v3: [Spike]Replace watch with nodemon (#1146) Fix Page Designer ImageWithText Link component (#1092) (#1148) Upgrade deprecated dependencies (#1124) Restart login flow if refresh_token is invalid (#1135) Move some util to site-utils to avoid circular imports (#1133) Restore old file name. (#1140) Update eslint configuration (#1129) CI: clarify the environment variables (#1127) Remove react-query-devtools from production build (#1121) Update lerna.json (#1118) Parallelize lighthouse ci (#1126) Increase test timeouts only on CI env (#1123) Remove unused `request` deprecated dependency. (#1125) Upgrade msw to latest (#1100) Add mergeBasket hook (#1114) [V3][Hooks Integration 🪝] Manually update cache for ShopperCustomer (#1113) dont use callback on mutateAsync (#1119) 2-spaces not 4-spaces (#1117) # Conflicts: # packages/internal-lib-build/package-lock.json # packages/pwa-kit-create-app/package-lock.json # packages/pwa-kit-dev/package-lock.json # packages/pwa-kit-dev/package.json # packages/pwa-kit-dev/src/configs/webpack/config.js # packages/pwa-kit-dev/src/ssr/server/build-dev-server.js # packages/pwa-kit-react-sdk/package-lock.json # packages/pwa-kit-runtime/package-lock.json # packages/template-express-minimal/package-lock.json # packages/template-mrt-reference-app/package-lock.json # packages/template-retail-react-app/package-lock.json # packages/template-typescript-minimal/package-lock.json # packages/test-commerce-sdk-react/package-lock.json

…-rehaul * feature/template-extensibility: [Spike]Replace watch with nodemon (#1146) Fix Page Designer ImageWithText Link component (#1092) (#1148) Upgrade deprecated dependencies (#1124) Restart login flow if refresh_token is invalid (#1135) Move some util to site-utils to avoid circular imports (#1133) Restore old file name. (#1140) Update eslint configuration (#1129) CI: clarify the environment variables (#1127) Remove react-query-devtools from production build (#1121) Update lerna.json (#1118) Parallelize lighthouse ci (#1126) Increase test timeouts only on CI env (#1123) Remove unused `request` deprecated dependency. (#1125) Upgrade msw to latest (#1100) Add mergeBasket hook (#1114) [V3][Hooks Integration 🪝] Manually update cache for ShopperCustomer (#1113) dont use callback on mutateAsync (#1119) 2-spaces not 4-spaces (#1117) # Conflicts: # packages/internal-lib-build/package-lock.json # packages/pwa-kit-dev/package-lock.json # packages/pwa-kit-react-sdk/package-lock.json # packages/pwa-kit-runtime/package-lock.json # packages/pwa-kit-runtime/package.json

vmarta added 2 commits April 12, 2023 12:46

Clarify environment variables

6944379

And also fixed the other `IS_DEFAULT_NPM` (the one for pwa-kit-windows). Related to PR #1126

Add comment

72bd990

vmarta commented Apr 12, 2023

View reviewed changes

vmarta marked this pull request as ready for review April 12, 2023 21:29

vmarta requested a review from a team as a code owner April 12, 2023 21:29

vmarta added the ready for review PR is ready to be reviewed label Apr 12, 2023

yunakim714 approved these changes Apr 13, 2023

View reviewed changes

wjhsf approved these changes Apr 13, 2023

View reviewed changes

vmarta merged commit ab4a499 into v3 Apr 14, 2023

vmarta deleted the CI-environment-variables branch April 14, 2023 04:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: clarify the environment variables #1127

CI: clarify the environment variables #1127

vmarta commented Apr 12, 2023 •

edited

Loading

vmarta Apr 12, 2023

vmarta Apr 12, 2023

wjhsf Apr 13, 2023

vmarta Apr 14, 2023

vmarta Apr 12, 2023 •

edited

Loading

wjhsf Apr 13, 2023

CI: clarify the environment variables #1127

CI: clarify the environment variables #1127

Conversation

vmarta commented Apr 12, 2023 • edited Loading

vmarta Apr 12, 2023

Choose a reason for hiding this comment

vmarta Apr 12, 2023

Choose a reason for hiding this comment

wjhsf Apr 13, 2023

Choose a reason for hiding this comment

vmarta Apr 14, 2023

Choose a reason for hiding this comment

vmarta Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

wjhsf Apr 13, 2023

Choose a reason for hiding this comment

vmarta commented Apr 12, 2023 •

edited

Loading

vmarta Apr 12, 2023 •

edited

Loading