search

shimming-toolbox / shimming-toolbox-matlab

Code for performing real-time shimming using external MRI shim coils
GNU General Public License v3.0
16 stars 5 forks source link

Set up CI and doc build with Azure Pipelines #81

Closed kousu closed 4 years ago

kousu commented 4 years ago

Sets up CI that can run against Matlab. We already have an Azure account set up with a self-hosted runner tristano.neuro.polymtl.ca with Matlab installed for https://github.com/qMRLab/qMRLab/.

So, this is #67 but using Azure instead of Github (which is sort of silly since Microsoft built both of them).

It would be nice to add tristano as a self-hosted Github runner because Github Actions is a lot easier to work with from Github than Azure -- but for this very large all-caps caveat:

Forks of your public repository can potentially run dangerous code on your self-hosted runner machine by creating a pull request that executes the code in a workflow.

Azure prompts for permission to be granted when a pipeline is first run on a given runner. Github Actions apparently hasn't implemented this yet? This seems bizarre; why would forks retain access to private runners? That's got to be wrong, or at least, in the process of becoming wrong. I don't want to be maintaining two CI infrastructures in parallel.

It might be useful to run on both Matlab and Octave, with some tests that depend on Matlab allowed to soft-fail on Octave (for comparison: pytest calls these xfails), since Octave can be parallelized easier and so will give faster feedback, but I don't really know yet; WIP WIP WIP.

kousu commented 4 years ago

Aha: https://github.community/t5/GitHub-Actions/Self-hosted-runner-security-with-public-repositories/m-p/50172#M7723

We can use tristano with Github Actions, but we have to make sure to not run them on PRs (the same way I disabled travis-ci/pr). PRs could run Octave on Github's infra, but tristano + matlab should be reserve to branches in the repo.

This does seem kind of fragile. It would be easy to open a cybersecurity risk since it fails-open instead of fails-closed.

jcohenadad commented 4 years ago

@kousu that sounds like a reasonable approach. We can add a big warning in the "contribution" and/or wiki to explain why github actions should be disabled for PRs (until this issue is resolved-- if it is one day). So, does it mean that we won't need to rely on Azure pipeline anymore? I like the idea of not having to learn another CI environment. It also means that we can choose a more performant station (although this bit could be reserved for later, if we find tristano is too slow).

agahkarakuzu commented 4 years ago

🎊 badge for this branch: Build Status

Badge for master (should look nice on README):

Build Status

agahkarakuzu commented 4 years ago

We can use tristano with Github Actions

@kousu @jcohenadad FYI, GitHub actions self-hosted runners do not have a good solution at an organization level yet.

On Azure, you can associate certain actions with approved users, that big warning on GitHub self hosted runners + [WIP] for organization repos were repelled me from giving them a try for the time being.

Equivalent of that warning on Azure:

https://docs.microsoft.com/en-us/azure/devops/pipelines/repos/github?view=azure-devops&tabs=yaml#validate-contributions-from-forks

agahkarakuzu commented 4 years ago

You should definitely check this checkbox:

image

On each PR, someone from the team can check files changed and confirm. I turned off fork builds for good measure anyway.

jcohenadad commented 4 years ago

@kousu i've extended the scope of this PR to also include doc build, hope that's ok

kousu commented 4 years ago

I think the doc build part should be isolated to https://github.com/shimming-toolbox/shimming-toolbox.org/pull/31 or we should merge these two repos.