LorenFrankLab / spyglass

Neuroscience data analysis framework for reproducible research built by Loren Frank Lab at UCSF
https://lorenfranklab.github.io/spyglass/
MIT License
94 stars 43 forks source link

No-credential data download #1180

Closed CBroz1 closed 1 week ago

CBroz1 commented 2 weeks ago

Description

This PR replaces the previous wget method of downloading the test data with curl.

By making these files public and switching the download method, we remove the need for credentials and can allow PRs to run the tests before merge, without presenting a security issue.

I made the choice for tests to run on approval. This prevents tests from running unnecessarily with each new commit on a open PR, and prevents un-reviewed code from being run. On the downside, it will adjust our workflow to separate approval from merge - requiring the reviewer to wait ~5m before merging. If we're on board with this, I can add branch protections that will prevent merge until the tests have passed.

Checklist:

edeno commented 2 weeks ago

I think I would prefer for the tests to run on commits because I think it helps catch early errors. I'm not sure there's a real reason to change it unless github is angry at us for running tests so frequently? @samuelbray32 do you have any thoughts or preferences?

samuelbray32 commented 2 weeks ago

If it doesn't cost us anything extra, I like having them run with commit a development feedback

CBroz1 commented 2 weeks ago

I'd still like to avoid runs of code from unknown sources. While we no longer have credentials exposed, I feel better knowing a submitter can't execute arbitrary code without some gatekeeping mechanism.

The latest commit requires that a PR have the 'RunTests' label to run the pytests. Only folks with repo write permissions can add labels, preventing outside contributors from running whatever they submit, but still allowing each of us to add the flag for our own PRs.

@edeno @samuelbray32 Does that work? I can add a reminder to the github template if that helps

samuelbray32 commented 2 weeks ago

That seems like a reasonable compromise to me

edeno commented 2 weeks ago

We already have these options enabled for actions. Do any of these assuage your concerns?:

image
CBroz1 commented 2 weeks ago

We already have these options enabled for actions. Do any of these assuage your concerns?

Oh, yes, they do - I must have missed these options in the resources I was reading