Revamp GitHub workflows using Ember.js native implementation #144

MrChocolatine · 2021-10-12T16:23:52Z

CI

Reset GitHub workflow file to Ember.js native implementation (#144)

Ember.js implementation:
https://github.com/ember-cli/ember-addon-output/blob/v4.0.0-beta.1/.github/workflows/ci.yml

Make things cleaner
Make things shorter
Make the PR Share logic in GitHub Actions with a composite Action #126 not needed anymore (logic less complex)

We used to manually handle the caching of dependencies while actions/setup-node natively handles it, with a few parameters, making our life easier.

One tiny addition compared to Ember.js implementation: jobs got a timeout of 20 minutes to avoid burning the default limit of 6 hours.

Remove scenario `ember-default` in `ember-try` (#144)

This scenario has been removed in ember-cli v3.20.0:
ember-cli/ember-addon-output@1ed3342

Update name of `ember-try` scenarios (#144)

To make them match with Ember.js native implementation and with the names usually used in the Ember.js ecosystem.

Fixes

Fix npm-script `lint` in `package.json` (#144)

See https://github.com/ember-cli/ember-cli/pull/9500/files

MrChocolatine · 2021-10-12T17:10:00Z

.github/workflows/ci.yml


 env:
-  NODE_VERSION: '12.x'
+  NODE_VERSION: '12'


The syntax 12 is enough if we just want to use the lastest major version.

MrChocolatine · 2021-10-12T17:11:21Z

.github/workflows/ci.yml

-
-    - name: Lint
-      run: yarn lint
+      - uses: actions/checkout@v2


This Action already uses fetch-depth: 1 as its default:
https://github.com/actions/checkout/blob/v2.3.4/README.md?plain=1#L92-L94

MrChocolatine · 2021-10-12T17:13:18Z

.github/workflows/ci.yml

+        uses: actions/setup-node@v2
+        with:
+          node-version: ${{ env.NODE_VERSION }}
+          cache: yarn


This allows us to let the Action handles the caching of dependencies:
https://github.com/actions/setup-node/blob/v2.4.1/docs/advanced-usage.md?plain=1#L45-L56

MrChocolatine · 2021-10-12T17:17:51Z

.github/workflows/ci.yml

-      with:
-        node-version: '${{ env.NODE_VERSION }}'
+      - name: Run Tests
+        run: yarn run test:ember


We used to run the following command: (source)

run: yarn test:ember --launch chrome

( ⬆️. because strategy.browser only has the value chrome)

I am not convinced --launch chrome is really needed, can someone confirm?

It seems useful when you have multiple browsers in testem.js launch_in_ci prop and you only want to run your test on one specific browser, as we only have Chrome in testem.js launch_in_ci prop, it seems ok to remove it

Noice, thanks.

MrChocolatine · 2021-10-12T17:18:21Z

.github/workflows/ci.yml


+      - name: Run Tests
+        run: yarn run test:ember


Same remark as https://github.com/peopledoc/ember-cli-embedded/pull/144/files#r727343324 .

Answer provided, see the related link ✅ .

ndekeister-us · 2021-10-13T12:32:35Z

.github/workflows/ci.yml

-      with:
-        node-version: '${{ env.NODE_VERSION }}'
+      - name: Run Tests
+        run: yarn run test:ember


It seems useful when you have multiple browsers in testem.js launch_in_ci prop and you only want to run your test on one specific browser, as we only have Chrome in testem.js launch_in_ci prop, it seems ok to remove it

ndekeister-us · 2021-10-13T12:48:55Z

.github/workflows/ci.yml


  try-scenarios:
-    name: Tests - ${{ matrix.ember-try-scenario }}
+    name: ${{ matrix.try-scenario }}


As we are updating this file, wondering if we should add timeout-minutes option, to avoid having issues with some scenarios running for 360 minutes (default value) because there is an issue in tests and no output is generated, see https://github.com/peopledoc/ember-cli-embedded/runs/3443628551?check_suite_focus=true for example

Was happening mainly for Beta/Canary but could be a "security" for "ember-release" scenario 🤷‍♂️

Indeed, I actually thought about it yesterday and forgot to add it, thanks.

https://github.com/ember-cli/ember-addon-output/blob/v4.0.0-beta.1/.github/workflows/ci.yml

To make them match with the names used in the GitHub workflows.

This was removed in `ember-cli` v3.20.0: ember-cli/ember-addon-output@1ed3342

Implemented in: https://github.com/ember-cli/ember-cli/pull/9500/files

I also moved the `needs` statements right after the `name` statements to sort them.

Inspired from: DazzlingFugu/ember-cli-embedded#144 Ember's implementation: https://github.com/ember-cli/ember-addon-output/blob/v4.0.0-beta.1/.github/workflows/ci.yml ci: add tests for Embroider compatibility

Inspired from: DazzlingFugu/ember-cli-embedded#144 Ember's implementation: https://github.com/ember-cli/ember-addon-output/blob/v4.0.0-beta.1/.github/workflows/ci.yml

MrChocolatine added the ci Continuous Integration label Oct 12, 2021

MrChocolatine force-pushed the revamp-gh-workflows branch 4 times, most recently from effd1e4 to 0d61c5b Compare October 12, 2021 16:50

MrChocolatine marked this pull request as ready for review October 12, 2021 17:02

MrChocolatine changed the title ~~revamp gh workflows~~ Revamp GitHub workflows using Ember.js native implementation Oct 12, 2021

MrChocolatine mentioned this pull request Oct 12, 2021

Share logic in GitHub Actions with a composite Action #126

Closed

MrChocolatine commented Oct 12, 2021

View reviewed changes

MrChocolatine requested a review from a team October 12, 2021 17:24

ndekeister-us approved these changes Oct 13, 2021

View reviewed changes

MrChocolatine added 11 commits October 13, 2021 14:01

ci: reset GH workflow to Ember's implementation

8671a1b

https://github.com/ember-cli/ember-addon-output/blob/v4.0.0-beta.1/.github/workflows/ci.yml

ci: manage Node.js version with an environment var

fce0ae8

ci: use yarn for cached dependencies

1b51717

ci: run commands with yarn over npm

7b972d8

ci: create a separate job lint

5b643a2

ci: remove testing scenarios beta and canary

6d4336f

refactor: space out things

312d539

ci(ember-try): update name of scenarios

1e8ff88

To make them match with the names used in the GitHub workflows.

ci(ember-try): remove scenario ember-default

154a8cd

This was removed in `ember-cli` v3.20.0: ember-cli/ember-addon-output@1ed3342

fix: fix syntax of npm-script lint

8d18648

Implemented in: https://github.com/ember-cli/ember-cli/pull/9500/files

ci: set jobs timeout to 20 minutes

cbe621c

I also moved the `needs` statements right after the `name` statements to sort them.

MrChocolatine force-pushed the revamp-gh-workflows branch from b637c7d to cbe621c Compare October 13, 2021 13:01

MrChocolatine merged commit 21c61e2 into master Oct 13, 2021

MrChocolatine deleted the revamp-gh-workflows branch October 13, 2021 13:10

MrChocolatine mentioned this pull request Oct 13, 2021

Release v2.1.0 #146

Merged

MrChocolatine added this to the Renovate GitHub workflows milestone Oct 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp GitHub workflows using Ember.js native implementation #144

Revamp GitHub workflows using Ember.js native implementation #144

MrChocolatine commented Oct 12, 2021 •

edited

Loading

MrChocolatine Oct 12, 2021

MrChocolatine Oct 12, 2021

MrChocolatine Oct 12, 2021

MrChocolatine Oct 12, 2021

ndekeister-us Oct 13, 2021

MrChocolatine Oct 13, 2021

MrChocolatine Oct 12, 2021

MrChocolatine Oct 13, 2021

ndekeister-us Oct 13, 2021

ndekeister-us Oct 13, 2021

MrChocolatine Oct 13, 2021

Revamp GitHub workflows using Ember.js native implementation #144

Revamp GitHub workflows using Ember.js native implementation #144

Conversation

MrChocolatine commented Oct 12, 2021 • edited Loading

CI

Reset GitHub workflow file to Ember.js native implementation (#144)

Remove scenario ember-default in ember-try (#144)

Update name of ember-try scenarios (#144)

Fixes

Fix npm-script lint in package.json (#144)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrChocolatine commented Oct 12, 2021 •

edited

Loading

Remove scenario `ember-default` in `ember-try` (#144)

Update name of `ember-try` scenarios (#144)

Fix npm-script `lint` in `package.json` (#144)