Maintenance: Improve CI for PRs

[dreamorosi]

Description of the improvement

Summary of the proposal

At the moment merging PRs requires maintainers to clone the branch locally, run tests, and then go back to GitHub to approve the PR.

To make it inclusive and accessible for contributors from different seniority, background and experience, this area should be be more detailed to make sure that ambiguity is reduced and most importantly, in case of PR's, expectations are communicated early on in the PR progress.

How, where did you look for information

Closed PRs, especially the ones from dependabot.

Missing or unclear documentation

N/A

Improvement

As discussed internally it would be beneficial to have an automation based on GitHub Actions that runs tests so that maintainers can see the results in the PR screen and make a decision.

Scripts like npm run lerna-ci to install the dependencies and npm run lerna-test in all the packages are already there.

Related existing documentation

NO

Related issues, RFCs

[dreamorosi]

The action used to setup node actions/setup-node@v2 recently released support for package caching but at this time it only works with package-lock.json and not npm-shrinkwrap.json which we are using.

There's an open issue on the action's repo saying that support for npm-shrinkwrap.json should be released in the coming months.

When this will be released we'll be able to add a key like this:

- uses: actions/setup-node@v2
  with:
    node-version: '14'
    cache: npm

Alternatively we could move back to package-lock.json but not sure it's worth it.

@saragerion thoughts?

[alan-churley]

I don't fully understand the ask here, the tests are already run against each PR automatically, and the results are shown on the PR page?

For example,

You can see here that the chore has passed all tests, and the feat:add tracer has not

If you go into the PR you can see more details, here the on-push-event runs all of our test suite

And if you click on details, in this example https://github.com/awslabs/aws-lambda-powertools-typescript/pull/107/checks?check_run_id=2973962167 you can see that this PR has for example failed linting

[dreamorosi]

Correct and apologies for not explaining it better, I was referring to PRs opened by Dependabot, like this one, that don't originate from a push but from a pull_request event.

It could be as simple as adding an event at after L3 of the current .workflows/on-push.yml file or having a dedicated workflow only for PRs, which I am open for discussion.

[alan-churley]

Ah I understand, thanks for explaining.

I think a separate workflow would be better personally, as we may want to do more on PR than on every push?

We will have to add some logic for dependabot PRs as they won't be able to push coverage reports for example, which we would still want for normal PRs. (https://github.blog/changelog/2021-02-19-github-actions-workflows-triggered-by-dependabot-prs-will-run-with-read-only-permissions/)

[github-actions]

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

[dreamorosi]

@alan-churley so it seems that my statement about Dependabot PRs was wrong and even though the results of checks didn't appear under the "Conversation" tab the the workflow still ran:

I think they didn't appear in the overview because I was looking at a closed PR, in the PR that was opened today (while still open)

A new PR #124 opened today had both on-push and on-pull-request workflows run.

If we want to avoid both running we could consider changing the on-push.yml to something like:

on:
  push:
    branches-ignore:
      - 'dependabot/**'