Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No-credential data download #1180

Merged
merged 12 commits into from
Nov 12, 2024
Merged

No-credential data download #1180

merged 12 commits into from
Nov 12, 2024

Conversation

CBroz1
Copy link
Member

@CBroz1 CBroz1 commented Nov 7, 2024

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:

  • No. This PR should be accompanied by a release: (yes/no/unsure)
  • N/a. If release, I have updated the CITATION.cff
  • No. This PR makes edits to table definitions: (yes/no)
  • N/a. If table edits, I have included an alter snippet for release notes.
  • N/a. If this PR makes changes to position, I ran the relevant tests locally.
  • I have updated the CHANGELOG.md with PR number and description.
  • N/a. I have added/edited docs/notebooks to reflect the changes

@CBroz1 CBroz1 marked this pull request as ready for review November 7, 2024 23:33
@CBroz1 CBroz1 marked this pull request as draft November 7, 2024 23:38
@CBroz1 CBroz1 marked this pull request as ready for review November 7, 2024 23:42
@edeno
Copy link
Collaborator

edeno commented Nov 8, 2024

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
Copy link
Collaborator

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

@CBroz1
Copy link
Member Author

CBroz1 commented Nov 8, 2024

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
Copy link
Collaborator

That seems like a reasonable compromise to me

@edeno
Copy link
Collaborator

edeno commented Nov 8, 2024

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

@CBroz1
Copy link
Member Author

CBroz1 commented Nov 8, 2024

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

Copy link
Collaborator

@edeno edeno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me other than the failing tests.

@CBroz1 CBroz1 requested a review from edeno November 12, 2024 19:39
@edeno edeno merged commit 239f2a5 into LorenFrankLab:master Nov 12, 2024
17 checks passed
@CBroz1 CBroz1 deleted the fta branch November 12, 2024 20:01
@CBroz1 CBroz1 mentioned this pull request Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants