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/**'

[alan-churley]

Personally I prefer having a separate on PR and on Push workflow, as it gives us more possibility going forward.

I think we need to fix the logic that prevents the On Push workflow running on PR as we currently stand as most of the logic is the same?

[dreamorosi]

Totally agree that they should be separate, and currently they are as one runs on: pull_request while the other on: push.

The problem is that my original assumption about Dependabot was wrong and what happens is that the bot pushes and opens the PR I rapid succession which causes the two workflows to run in the same batch/check.

I've been investigating but so far the easiest way to avoid this is to ignore all branches that start with dependabot/* in the push workflow.

@alan-churley what do you think?

[heitorlessa]

FYI - we have access to Mergify, you just need a ticket (ping me tomorrow). I had issues with auto merge as major and minor filters weren’t that flexible — right now I approve and gets merged if it’s a dependabot PR, and do-not-merge labelled PRs are blocked from merge as well… not all dependencies follow semver strictly

…

On Thu, 22 Jul 2021 at 19:49, Andrea Amorosi ***@***.***> wrote: Totally agree that they should be separate, and currently they are as one runs on: pull_request while the other on: push. The problem is that my original assumption about Dependabot was wrong and what happens is that the bot pushes and opens the PR I rapid succession which causes the two workflows to run in the same batch/check. I've been investigating but so far the easiest way to avoid this is to ignore all branches that start with dependabot/* in the push workflow. @alan-churley <https://github.com/alan-churley> what do you think? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#120 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZPQBCTUACSBLDFA2LHE5LTZBKZDANCNFSM5AVPH55A> .

[alan-churley]

@dreamorosi what I am actually suggesting is that we don't want the On Push to run on any PRs, not just the Dependant, as they are essentially running the same tests

[dreamorosi]

@alan-churley I see and I misunderstood the way these events/workflow work. As you suggested in our conversation offline we need to look into it more & it's not the end of the world if both run.

We might also explore what the Python repo is doing and/or look into Mergify as Heitor suggests.

[dreamorosi]

Going to close this issue as CI for PRs has already been addressed, Dependabot's PR can be addressed with a separate issue at a later time.

[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.