jelhan/create-github-actions-setup-for-ember-addon

Setup GitHub Actions for an Ember Addon

create-github-actions-setup-for-ember-addon's Issues

Improve readability of created workflow

@ijlee2 proposed to add empty lines between each step and after each job to improve readability:

It may also help to add 1 empty line after each step and 2 empty lines after each job. I find that this helps me read a workflow more easily.

See jelhan/ember-style-modifier#32 (comment) for more context.

Simplify node version upgrades by defining it once in created workflow

@ijlee2 pointed out that we can define the node version once as a global variable and use it at many places in the workflow: jelhan/ember-style-modifier#32 (comment) This would ease upgrades of node version in generated CI workflow.

Support TravisCI configuration created by Ember CLI <= v3.3.0

Ember CLI changed the format used in TravisCI configuration file with its v3.4.0 release. Some addons still use the format generated by Ember CLI <= v3.3.0.

The main difference between both is the matrix definition for the Ember Try Scenarios.

This is the matrix definition used in v3.3.0 and before:

env:
  global:
    # See https://git.io/vdao3 for details.
    - JOBS=1
  matrix:
    # we recommend new addons test the current and previous LTS
    # as well as latest stable release (bonus points to beta/canary)
    - EMBER_TRY_SCENARIO=ember-lts-2.12
    - EMBER_TRY_SCENARIO=ember-lts-2.16
    - EMBER_TRY_SCENARIO=ember-lts-2.18
    - EMBER_TRY_SCENARIO=ember-release
    - EMBER_TRY_SCENARIO=ember-beta
    - EMBER_TRY_SCENARIO=ember-canary
    - EMBER_TRY_SCENARIO=ember-default

Since v3.4.0 Ember CLI generates a Travis CI configuration file defining the Ember Try Scenario matrix like this:

jobs:
  fail_fast: true
  allow_failures:
    - env: EMBER_TRY_SCENARIO=ember-canary

  include:
    # runs linting and tests with current locked deps

    - stage: "Tests"
      name: "Tests"
      script:
        - npm run lint:hbs
        - npm run lint:js
        - npm test

    # we recommend new addons test the current and previous LTS
    # as well as latest stable release (bonus points to beta/canary)
    - stage: "Additional Tests"
      env: EMBER_TRY_SCENARIO=ember-lts-2.12
    - env: EMBER_TRY_SCENARIO=ember-lts-2.16
    - env: EMBER_TRY_SCENARIO=ember-lts-2.18
    - env: EMBER_TRY_SCENARIO=ember-release
    - env: EMBER_TRY_SCENARIO=ember-beta
    - env: EMBER_TRY_SCENARIO=ember-canary
    - env: EMBER_TRY_SCENARIO=ember-default-with-jquery

To support the older format as well the parser for TravisCI configuration files needs to be improved.

[Feature Request] Support Node versions <14

Currently, the engines property in the package.json is set to >14
This means that Node versions v12, for example, don't work.

https://github.com/jelhan/create-github-actions-setup-for-ember-addon/blob/master/package.json#L49

To get around this I switched to Node version v14 using NVM.

Consider running more jobs in parallel

@ijlee2 proposed that we should run more jobs in parallel:

3. I believe, currently, the workflow uses parallelization in the following manner. Step 2 can occur only when Step 1 succeeds, Step 3 only when Step 2, etc.

1. lint
2. test, floating-dependencies
3. try-scenarios

For addons (public repos, free of charge), a maintainer may want to run everything in parallel:

## An alternative
1. lint, test, floating-dependencies, try-scenarios

## Another alternative
1. lint
2. test, floating-dependencies, try-scenarios

Just something to consider. slightly_smiling_face

For more context see jelhan/ember-style-modifier#32 (comment), jelhan/ember-style-modifier#32 (comment) and jelhan/ember-style-modifier#32 (comment).

The TravisCI configuration created by Ember CLI addon blueprints run the jobs in two stages:

1. lint and test, which is even run in the same job.
2. Floating dependencies and ember try scenarios.

Not sure if we should aim to replicate that or run everything in parallel as we don't have constrains on CI minutes.

GitHub Additional Product Terms include a section about GitHub Actions, which forbids "any activity that places a burden on our servers, where that burden is disproportionate to the benefits provided to users". I think we can argue that having quicker CI results and getting detailed failure reports for all scenarios even if linting or default tests are failing, may outweigh the additional load on the servers.

Not sure if it may speed up overall executing time if one quick job (lint) prepopulates the dependency cache for all following jobs. This may also only be the case if dependencies are changed in the pull request. Otherwise cache may be prepopulated before by runs for other pull requests or master branch.

Use reasonable defaults

The defaults currently used do not make much sense. Just used the once from GitHub Actions workflow I used as a blueprint. Should use reasonable defaults instead. Maybe even depending on the ember-cli version of the project.

Support custom code coverage checks in CI

I noticed that some adopted addons, which are not yet migrated to GitHub Actions, added code coverage checks to their TravisCI pipeline. Would be great if we could support this use case out of the box.

• ember-stripe-elements: https://github.com/adopted-ember-addons/ember-stripe-elements/blob/7c5c8ef681a2d507293d8dc4661fd0d4d2c50da7/.travis.yml#L49-L50
• ember-light-table: https://github.com/adopted-ember-addons/ember-light-table/blob/69b25a6333e5784d4ce56dd3c4dcc30cc3d145fa/.travis.yml#L53-L54
• ember-file-upload: https://github.com/adopted-ember-addons/ember-file-upload/blob/556075f97c2ceada0ac11c3bcfbd734b84bd6245/.travis.yml#L42-L43

Created workflow fails if project does not have lint and test:ember scripts

Ember CLI blueprints doesn't had lint and test:ember scripts before 3.18: ember-cli/ember-addon-output@0be9a36 But created workflow rely on them being present.

One solution would be adding that script to package.json and installing npm-run-all if not present yet. As an alternative lint:hbs and lint:js could be run as separate steps if project does not have a lint script.

Simplify cache setup in created CI workflow

Created CI workflow currently uses three steps to setup the cache of node_modules and global yarn or npm cache:

create-github-actions-setup-for-ember-addon/templates/.github/workflows/ci.yml

Lines 20 to 53 in e518896

	- name: Get package manager's global cache path
	id: global-cache-dir-path
	run: echo "::set-output name=dir::$(<%
	if (packageManager === 'npm') {
	%>npm config get cache<%
	} else {
	%>yarn cache dir<%
	} %>)"
	- name: Cache package manager's global cache
	id: cache-global-package-manager-cache
	uses: actions/cache@v1
	with:
	path: ${{ steps.global-cache-dir-path.outputs.dir }}
	key: ${{ runner.os }}-${{ matrix.node-version }}-yarn-${{ hashFiles('**/<%
	if (packageManager === 'npm') {
	%>package-lock.json<%
	} else {
	%>yarn.lock<%
	} %>') }}
	restore-keys: |
	${{ runner.os }}-${{ matrix.node-version }}-yarn-
	- name: Cache node_modules
	id: cache-node-modules
	uses: actions/cache@v1
	with:
	path: node_modules
	key: ${{ runner.os }}-${{ matrix.node-version }}-nodemodules-${{ hashFiles('**/<%
	if (packageManager === 'npm') {
	%>package-lock.json<%
	} else {
	%>yarn.lock<%
	} %>') }}
	restore-keys: |
	${{ runner.os }}-${{ matrix.node-version }}-nodemodules-

@ijlee2 pointed out in a review that it can be simplified to two steps with less code. It will also allow to simplify the if statement used in dependency installation: jelhan/ember-style-modifier#32 (comment)

Created CI workflows should cache dependencies

Created CI workflows should cache dependencies. One solution is described by @isaac in I Want to Cache chapter of his blog post CI with GitHub Actions for Ember Apps.

Support updating a created workflow

It would be great if create-github-actions-setup-for-ember-addon can update a GitHub Actions workflow created in a previous run.

One way to do so would be storing the configuration used to generate the CI workflow. create-github-actions-setup-for-ember-addon has a configuration interface, which describes the configuration object used to create the CI workflow template:

create-github-actions-setup-for-ember-addon/src/index.ts

Lines 8 to 16 in a95d213

	interface ConfigurationInterface {
	browsers: string[];
	emberTryScenarios: {
	allowedToFail: string[];
	required: string[];
	};
	nodeVersion: string;
	packageManager: 'npm' | 'yarn';
	}

The parser for TravisCI configuration returns the extracted configuration in that format. The default configuration if no TravisCI configuration also uses that format.

The configuration object used to generate a workflow could be stored at top of the generated CI workflow. It should be stored as a YAML comment to not conflict with the workflow definition itself. It should be stored in a human readable format like YAML. Maybe it's just an outcommented YAML?

# This file has been generated by `yarn create github-actions-setup-for-ember-addon`
# using the following configuration:
#
# browsers:
#   - chrome
# emberTryScenarios:
#   allowedToFail:
#     - ember-beta
#     - ember-canary
#   required:
#     - ember-lts-3.20
#     - ember-release
# nodeVersion: 10
# packageManager: yarn
#
# Running `yarn create github-actions-setup-for-ember-addon` again will update the
# CI workflow to the latest blueprint.

We would than implement a parser similar to the parser for TravisCI configuration, which looks for a .github/workflows/ci.yml and extract the information from previous run out of it.

Maybe we want to store a version together with the configuration object if we want to ship a breaking change in that configuration object.

CI fails for Firefox

CI configuration does not seem to be correct for Firefox. Seeing a repository migrated from TravisCI with CI configured to run both on Chrome and Firefox fail with:

 not ok 1 Firefox - [undefined ms] - error
    ---
        message: >
            Error: Browser exited unexpectedly
            Non-zero exit code: 1
            Stderr: 
             Error: no DISPLAY environment variable specified
            
            
        browser log: |
            [object Object]
            [object Object]
            [object Object]
    ...

See this job as an example.

Provide better feedback if package manager used in .travis-ci.yml and lock file does not match

Some repositories seem to have a mismatch between the package manager used in TravisCI configuration (.travis-ci.yml) and the lock file committed to the repository.

@simonihmig reported this issue when migrating ember-responsive-image. The .travis-ci.yml was using npm but a yarn.lock was committed to the repository.

I faced the same issue when migrating ember-leaflet. But in this case the .travis-ci.yml was using yarn but a package-lock.json was committed to the repository.

It seems like common that TravisCI configuration does not use the package manager expected by the project. I guess the project changed the package manager at some point of time but missed to update .travis-ci.yml and Ember CLI Update configuration.

In most cases a mismatch between package manager used in TravisCI configuration and expected by the project is not an issue. TravisCI picks correct package manager automatically unless install step is provided explicitly. TravisCI configuration created by Ember CLI blueprints only overwrites the install step for the Floating Dependencies scenario. As lock file should be ignored for that scenario using the wrong package manager does not cause any harm.

I think package manager should be detected based on existing yarn.lock or package-lock.json. Even if migrating an existing TravisCI configuration. Existing yarn.lock or package-lock.json seems to be the source of truth. I don't see a use case for using another package manager in CI. Relying on TravisCI configuration and not existing lock file does not seem to provide any value. But it has the risk to migrate bugs...

Empty `include:` statement generated

An empty include: statement was generated here, which makes the CI file invalid.

Also, it seems ember-canary wasn't allowed to fail, so I'm guessing that's related?

Consider to support deployment step

I noticed that some adopted ember addons have a custom deployment step at the end of their pipeline:

• ember-autoresize: https://github.com/adopted-ember-addons/ember-autoresize/blob/e1d172f69c89772a5d48dabf340335e932a09619/.travis.yml#L47-L50
• ember-collapsile-panel: https://github.com/adopted-ember-addons/ember-collapsible-panel/blob/e6bc55477761df5d57de5981c9d97c5ffb5c632c/.travis.yml#L33-L40

This is a similar use case as having a code coverage check, which is tracked in #40. But while it's quiet clear that code coverage belongs to CI, I'm actually not that sure for deployment. Unless the version is also released in the CI pipeline, one might argue that the docs for a released version should be deployed regardless if CI passes or not. Especially if that CI does not verify the documentation app itself. In that case a separate workflow might be better.

Cache key contains word "yarn" even if NPM is used

Noticed by @ijlee2 in onechiporenko/ember-bootstrap-modals-manager#28 (comment). It should not fail the workflow. But is very likely to cause confusion. Will consider it as a bug therefore.

Allow user to select options using an interactive mode

An interactive mode driving users through CI creation would be great. Currently it's too much magic without feedback and manual intervention in my perspective.

This could be done using a library like enquire.

We might just start with one question for the beginning:

1. How to derive the values for your CI pipeline? [select]
   • Use configuration from previous run
   • Migrate existing TravisCI pipeline
   • Calculate default values based on the project

Later we might add an option to customized derived options:

2. We calculated this configuration for you:
   Package manager: yarn
   Node version: 12
   Ember Try scenarios:

   • ember-lts-3.16
   • ember-lts-3.20
   • ember-release
   • ember-beta
   • ember-canary
   • ember-default-with-jquery
   • ember-classic

   Do you want to tweak this configuration? [y/N]

Support arbitary environment variables in job configuration

So far only EMBER_CLI_SCENARIO environment variable is extracted from .travis.yml configuration and used to generate the GitHub Action CI:

create-github-actions-setup-for-ember-addon/src/utils/parse-travis-ci-config.ts

Lines 65 to 74 in e518896

	const emberTryScenarios: string[] = config.jobs.include
	.map(({ env }: { env: unknown }): string | null => {
	if (typeof env !== 'string') {
	return null;
	}
	const [key, value] = env.split('=');
	return key === 'EMBER_TRY_SCENARIO' ? value : null;
	})
	.filter((_: string | null) => _ !== null);

create-github-actions-setup-for-ember-addon/templates/.github/workflows/ci.yml

Lines 204 to 207 in e518896

	ember-try-scenario: [<%
	emberTryScenarios.required.forEach(function(scenario, index){ %>
	<%- scenario %><% if (index < emberTryScenarios.required.length - 1) { %>,<% }}); %>
	]

It should be possible to migrate every environment variable defined in TravisCI in that way to GitHub Actions. Instead of ignoring all environment variables, which aren't EMBER_CLI_SCENARIO, we could use a key value map to store them and loop over it in the GitHub Actions creation.

We would need to change the configuration used to generate GitHub Actions.

create-github-actions-setup-for-ember-addon/src/index.ts

Lines 33 to 48 in e518896

	const data: ConfigurationInterface = parseTravisCiConfig() ?? {
	browsers: ['chrome', 'firefox'],
	emberTryScenarios: {
	required: [
	'ember-lts-3.16',
	'ember-lts-3.20',
	'ember-release',
	'ember-beta',
	'ember-default-with-jquery',
	'ember-classic',
	],
	allowedToFail: ['ember-canary', 'embroider-tests'],
	},
	nodeVersion: '10.x',
	packageManager: 'yarn',
	};

Maybe something like this:

 const data: ConfigurationInterface = parseTravisCiConfig() ?? {
   browsers: ['chrome', 'firefox'],
-  emberTryScenarios: {
-    required: [
-      'ember-lts-3.16',
-      'ember-lts-3.20',
-      'ember-release',
-      'ember-beta',
-      'ember-default-with-jquery',
-      'ember-classic',
-    ],
-    allowedToFail: ['ember-canary', 'embroider-tests'],
-  },
+ jobs: [
+   {
+     environmentVariables: {
+       EMBER_CLI_SCENARIO: 'ember-lts-3.16',
+     },
+     allowedToFail: false,
+   },
+   {
+     environmentVariables: {
+       EMBER_CLI_SCENARIO: 'ember-lts-3.20',
+     },
+     allowedToFail: false,
+   },
+   {
+     environmentVariables: {
+       EMBER_CLI_SCENARIO: 'ember-release',
+     },
+     allowedToFail: false,
+   },
+   {
+     environmentVariables: {
+       EMBER_CLI_SCENARIO: 'ember-beta',
+     },
+     allowedToFail: false,
+   },
+   {
+     environmentVariables: {
+       EMBER_CLI_SCENARIO: 'default-with-jquery',
+     },
+     allowedToFail: false,
+   },
+   {
+     environmentVariables: {
+       EMBER_CLI_SCENARIO: 'ember-canary',
+     },
+     allowedToFail: true,
+   },
+   {
+     environmentVariables: {
+       EMBER_CLI_SCENARIO: 'embroider-tests',
+     },
+     allowedToFail: true,
+   },
+ ]
   nodeVersion: '10.x',
   packageManager: 'yarn',
 };

npm/yarn detection not working?

I ran this tool on https://github.com/kaliber5/ember-responsive-image, and it created the npm-based workflow, although there is a yarn.lock! 🤔
Haven't looked further into it yet...

Add contributing documentation

Add a CONTRIBUTING.md. It should contain all relevant information to ease contributions to this repository. Especially it should explain:

1. The ESJ template located in templates/.github/workflows/ci.yml used to generate the workflow.
2. The concept of parser that extract the configuration from existing files (currently only TravisCI configuration is supported through src/utils/parse-travis-ci-config.js).
3. Snapshot testing with jest: How it works, how to update a snapshot (yarn test -u) and how to verify that changes are as expected.
4. Fixtures located in tests/fixtures used as test scenarios.

See #15 (comment) as an example of a question that should be answered by contributing docs.

Hardcoded master branch instead of using the travis branches config

create-github-actions-setup-for-ember-addon/templates/.github/workflows/ci.yml

Line 23 in cdc8b63

- master

I'm using main.

CI runs twice if pull request is created from same repository

CI is triggered both for push and pull_request if a pull request is created from a feature branch within the same repository (not a fork). See this pull request as an example.

This is likely an issue with this configuration:

create-github-actions-setup-for-ember-addon/templates/.github/workflows/ci.yml

Line 3 in ee1d848

on: [push, pull_request]

Consider to only cache package manager global cache or node_modules not both

In #13 it was proposed to not cache both package manager's global cache and node_modules but only one of them.

@simonihmig proposed to cache only package manager's global cache:

What it does not do however AFAIK is that it caches both the npm/yarn cache as well as node_modules itself (it does only the former). But what I am really not sure about is how useful caching both is. The former will already prevent fetching from the network I believe. And the examples given by @actions/cache also show only the former version of caching: https://github.com/actions/cache/blob/main/examples.md#node---yarn 🤷‍♂️

@kategengler proposed to cache only node_modules:

In my experimentation, the best caching I've found was by caching node_modules/ See emberjs/ember.js#19098

I think additionally to what to cache we should discuss when to reuse the cache. It might be quicker to always reuse the same cache even if yarn.lock or package-log.json changes as installation is much faster if node_modules is prepopulated.

I fear we won't be able to answer these questions without benchmarks and some real world experience. Not sure if investing that time is worth the performance win to be honest. But maybe there are already good articles on that topic?