Create physionet-build Github CI action

[briangow]

[WIP] This github action will replace the shippable.yml for build-test.

This action is my initial attempt to replicate the process that has been used in the shippable.yml with Github actions. Please do not merge it yet.

I've added descriptions for what the code is doing in the -name: fields. If you could review these and suggest corrections it would be appreciated.

Regarding the lines of code starting with coverage. This code is designed to report on how much of the code was actually executed: https://coverage.readthedocs.io/en/coverage-5.5/ . Do we want to keep doing these coverage checks? If so, is it sufficient to report the result in the log instead of saving it to a file as was done in shippable.yml?

[Lucas-Mc]

Exit with code 127 source: not found ... maybe . env/bin/activate since source isn't part of /bin/sh

EDIT
Here's another way:

defaults:
  run:
    shell: bash

[tompollard]

You should be able to fix the postgres error by adding postgres as a service:

    services:
      postgres:
        image: postgres:11
        env:
          POSTGRES_DB: physionet        
          POSTGRES_PASSWORD: password
          POSTGRES_USER: physionet
        ports:
          - 5432:5432
        # Set health checks to wait until postgres has started
        options: >-
          --health-cmd pg_isready
          --health-interval 10s
          --health-timeout 5s
          --health-retries 5

The manual installation can then be removed:

• remove apt-get install postgresql
• remove service postgresql start
• remove the setup postgres chunk

[bemoody]

On Wed, 14 Apr 2021 20:45:33 -0700, Tom Pollard ***@***.***> wrote: You should be able to fix the postgres error by adding postgres as a service:

I think that is not a good idea. (a), we want to test against the Debian stable version of postgres, not whatever version is supplied by github. And (b), this script should be self-contained, not dependent on the provider. We need to be able to spin up a virtual machine of our own and run the same exact commands, and it should give exactly the same results as the automated test. (The problem at c182dd5 is that you're not invoking the psql commands as the "postgres" user. You need to invoke those using sudo -u postgres.) It seems it's impossible to view the logs without being logged in to Github? That's a serious problem.

[tompollard]

I think that is not a good idea. (a), we want to test against the
Debian stable version of postgres, not whatever version is supplied by
github.

Yep, fair point!

[Lucas-Mc]

What other options do we have? GHA seems a little restrictive but I think it can get close enough without being exactly the same as how it previously was. If this is going to be shot down then we need an alternative.

…

On Thu, Apr 15, 2021 at 12:47 Tom Pollard ***@***.***> wrote: I think that is not a good idea. (a), we want to test against the Debian stable version of postgres, not whatever version is supplied by github. Yep, fair point! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1320 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJNM7BSDJ4ZGPJ6WPAGOY43TI4KDPANCNFSM4256BVBA> .

[tompollard]

What other options do we have?

I think we go back to the original approach, which is to install and start a postgres service on the server.

[bemoody]

I think we go back to the original approach, which is to install and start a postgres service on the server.

Yeah, that ought to work if we get the permissions correct. Brian, you said in ad7dead that Docker runs as root; I'm not sure if that's true or not, but either way, there are some commands (like installing lightwave) that require root privileges, while other commands (e.g. manage.py test) specifically *should* be run as a non-root user so that we know the permission checks work correctly.

[tompollard]

Brian, you said in ad7dead that Docker runs as root; I'm not sure if that's true or not, but either way, there are some commands (like installing lightwave) that require root privileges, while other commands (e.g. manage.py test) specifically should be run as a non-root user so that we know the permission checks work correctly.

In this case, we probably need to add a step at the beginning of the process to create some kind of build user. Seemed strange to be running everything as root anyway.

[briangow]

I'll look into setting up a separate user from root. @bemoody , do we need to store a file for the logs?

[bemoody]

I don't really know Docker, I've only used it occasionally, but I'd be a little surprised if Docker (or Github) didn't set up an unprivileged account within the container by default. I'm not going to worry too much about the logs for now. I think we would be better off in a lot of ways running these tests on our own servers (e.g. using jenkins or buildbot) but whether this is more or less work, overall, than dealing with the annoyances of Github, I'm not sure.

[briangow]

@bemoody I confirmed with whoami in the Docker container that it is running as root. This indicates that we should only be running as root when using a Docker container inside Github actions:

"USER
Docker actions must be run by the default Docker user (root). Do not use the USER instruction in your Dockerfile, because you won't be able to access the GITHUB_WORKSPACE "
https://docs.github.com/en/actions/creating-actions/dockerfile-support-for-github-actions

[bemoody]

I see. I am not sure what they mean exactly about accessing GITHUB_WORKSPACE. Here somebody suggests specifying a UID in container.options: https://github.community/t/how-to-run-action-as-a-non-root-user/17572 (why 1001, though, I have no idea.)

[briangow]

Yeah, I'd also seen that. I wasn't able to figure out how to create a non-root user before issuing the --user command and have the created user be seen at the options: --user line. I thought this would be helpful since they also needed to do a "pre-step" before calling their Docker container setup: actions/runner#812 . However, I haven't been able to successfully setup a Docker container under the steps: section.

This gives a bit of information about GITHUB_WORKSPACE:

GITHUB_WORKSPACE | The GitHub workspace directory path. The workspace directory is a copy of your repository if your workflow uses the actions/checkout action. If you don't use the actions/checkout action, the directory will be empty. For example, /home/runner/work/my-repo-name/my-repo-name.
https://docs.github.com/en/actions/reference/environment-variables

I can look into other options for checking out the repository without using the checkout action.

[briangow]

I can run a given command (whoami in this case) as a non-root user with:
sudo -u physionet_user whoami

Also, I realized that the choice of --user 1001 from this thread: https://github.community/t/how-to-run-action-as-a-non-root-user/17572/2 , wasn't a random choice as /home/1001 already exists. However, since this account doesn't have privileges I can't do much with it after issuing --user 1001.

[bemoody]

I can run a given command (`whoami` in this case) as a non-root user with: `sudo -u physionet_user whoami`

Maybe we could do something like this? - name: create an unprivileged user account run: | /sbin/useradd pntestuser chown pntestuser.pntestuser -R . ... - name: setup and test physionet, check amount of code tested shell: sudo -u pntestuser bash -e {0} run: | python physionet-django/manage.py makemigrations --dry-run --no-input --check python physionet-django/manage.py resetdb python physionet-django/manage.py loaddemo python physionet-django/manage.py test --verbosity=3 --keepdb coverage run --source='.' physionet-django/manage.py test --keepdb coverage report -m Though sudo might also monkey with environment variables like PATH...

[briangow]

Thanks @bemoody , your suggestion does work for setting up a pntestuser and switching to that user for a given run:

I'm still trying to work out if it's possible to clone physionet-build to a location that is not under $GITHUB_WORKSPACE since we can only access that workspace as root (which I confirmed). Instead of using the github action checkout@v2, I installed git and ran:

      - name: clone physionet-build
        run: |
          git clone https://github.com/MIT-LCP/physionet-build.git

running ls -la $GITHUB_WORKSPACE
gives:

total 12
drwxr-xr-x 3 pntestuser pntestuser 4096 Apr 17 02:28 .
drwxr-xr-x 3       1001        121 4096 Apr 17 02:27 ..
lrwxrwxrwx 1 root       root         12 Apr 17 02:28 .env -> .env.example
drwxr-xr-x 7 root       root       4096 Apr 17 02:28 physionet-build

[briangow]

@bemoody , from what I can tell Github actions doesn't allow access to cloned repositories (in the GITHUB_WORKSPACE) while running as a non-root user. This post explains some of the restrictions around not being able to clone outside of the GITHUB_WORKSPACE:

"Today the path input is the only way to change the directory where the code is cloned. Also there is a restriction that it must be under GITHUB_WORKSPACE."
actions/checkout#197

As mentioned previously, you can't access the GITHUB_WORKSPACE unless you are root. A workaround would be to use a self-hosted runner but I don't think we want to do that. I could ask in a forum to check to see if there is a way around this that I haven't been able to find so far.

[bemoody]

I think this will probably work but we need to think seriously about how to improve the testing workflow. It looks like neither these nor the shippable tests are run when the dev branch is updated, so the final result isn't being tested until *after* it's merged. Of course we still can test things after merging into dev and before deploying, but I feel like it would be better for the pull request to be blocked until it's been fully tested. However, unless you see any easy fixes, I think this meta discussion should happen separately.

[tompollard]

As this is currently just supplementing existing tests, I'd be good to merge now if everyone else is okay with that? We can then add new issues for (1) fixing the user issue and (2) triggering rebuilds on pull requests when the target is updated.

[Lucas-Mc]

It looks like neither these nor the shippable tests are run when the dev branch is updated, so the final result isn't being tested until after it's merged.

Can you explain more of what you mean by that? This:

on:
   push:
     branches:
       - dev
   pull_request:
     branches:
       - dev

should be triggering tests whenever a commit is pushed to dev or a pull request is targeting dev (new commits to the pull request also trigger tests).

Are you saying we require all checks to pass before it can be merged? I think this is just a protected branch issue and not GHA issue.

[bemoody]

Can you explain more of what you mean by that? This: ``` on: push: branches: - dev pull_request: branches: - dev ``` should be triggering tests whenever a commit is pushed to `dev` or a pull request is targeting `dev` (new commits to the pull request also trigger tests).

If I understand correctly, it runs in two cases: - When a commit is pushed to dev, it runs the tests on the current state of dev. This has no effect on pull requests and doesn't take pull requests into account. - When a pull request is created, or a commit is pushed to a branch linked to a pull request, it tests the result of merging that commit with the *current* state of dev. So when there are two or more pull requests at once, and you merge one of them and push it to dev, it doesn't re-test the other branches against the new state of dev. For example, here, it appears to have run tests for - briangow-patch-3~1 (454b121) with dev~1 (e0f883c) (merged as 5ee9aa2, tested by run 755909966) - briangow-patch-3 (77f1768) with dev (2639176) (merged as 314098a, tested by run 768130278) but after 2639176 was pushed to dev it never tested the result of merging 454b121 with 2639176.

[bemoody]

Improvements are welcome but let's discuss them elsewhere. :)