-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Migrate from Azure to GH Actions #1306
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
Merged
Merged
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
7f9ea69
add pytest workflow
kandersolar aa4cc95
add some comments
kandersolar cc993b1
set fail-fast to false
kandersolar e691532
see if secrets work
kandersolar 99446a0
move --remote-data tests to other workflow
kandersolar 9463f0b
remove secrets from default pytest workflow
kandersolar 584f134
simplify bare environment installation
kandersolar 4c2e755
simplify
kandersolar 23416dc
un-simplify
kandersolar 7846f0e
thrashing around the "debug the CI" morass
kandersolar 2e6eda3
fix macos py3.6
kandersolar 3d4f0f7
disable codecov upload in azure pipelines
kandersolar d5bac80
enable codecov in github actions
kandersolar 7ec9f65
fix janky coverage hack
kandersolar 8298308
undo unnecessary changes to .coveragerc
kandersolar fef33d5
add label reminder to PR template
kandersolar e50b310
rework workflows and codecov
kandersolar bf35da0
Merge remote-tracking branch 'upstream/master' into github_actions
kandersolar a15b528
whatsnew
kandersolar b3b9c7d
remove azure config files
kandersolar d5275ce
don't pytest on all pushes, only master
kandersolar ea17229
build sdist on PRs
kandersolar 4b78783
switch to more convenient environment filenames
kandersolar 5a486fb
use most recent checkout action
kandersolar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
# A secondary test job that only runs the iotools tests if explicitly requested | ||
# (for pull requests) or on a push to the master branch. | ||
# Because the iotools tests require GitHub secrets, we need to be careful about | ||
# malicious PRs accessing the secrets and exposing them externally. | ||
# | ||
# We prevent this by only running this workflow when a maintainer has looked | ||
# over the PR's diff and verified that nothing malicious seems to be going on. | ||
# The maintainer then adds the "remote-data" label to the PR, which will then | ||
# trigger this workflow via the combination of the "on: ... types:" | ||
# and "if:" sections below. The first restricts the workflow to only run when | ||
# a label is added to the PR and the second requires one of the PR's labels | ||
# is the "remote-data" label. Technically this is slightly different from | ||
# triggering when the "remote-data" label is added, since it will also trigger | ||
# when "remote-data" is added, then later some other label is added. Maybe | ||
# there's a better way to do this. | ||
# | ||
# But wait, you say! Can't a malicious PR get around this by modifying | ||
# this workflow file and removing the label requirement? I think the answer | ||
# is "no" as long as we trigger the workflow on "pull_request_target" instead | ||
# of the usual "pull_request". The difference is what context the workflow | ||
# runs inside: "pull_request" runs in the context of the fork, where changes | ||
# to the workflow definition will take immediate effect, while "pull_request_target" | ||
# runs in the context of the main pvlib repository, where the original (non-fork) | ||
# workflow definition is used instead. Of course by switching away from the fork's | ||
# context to keep our original workflow definitions, we're also keeping all the | ||
# original code, so the tests won't be run against the PR's new code. To fix this | ||
# we explicitly check out the PR's code as the first step of the workflow. | ||
# This allows the job to run modified pvlib & pytest code, but only ever via | ||
# the original workflow file. | ||
# So long as a maintainer always verifies that the PR's code is not malicious prior to | ||
# adding the label and triggering this workflow, I think this should not present | ||
# a security risk. | ||
# | ||
# Note that this workflow can be triggered again by removing and re-adding the | ||
# "remote-data" label to the PR. | ||
# | ||
# Note also that "pull_request_target" is also the only way for the secrets | ||
# to be accessible in the first place. | ||
# | ||
# Further reading: | ||
# - https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ | ||
# - https://github.community/t/can-workflow-changes-be-used-with-pull-request-target/178626/7 | ||
|
||
name: pytest-remote-data | ||
|
||
on: | ||
pull_request_target: | ||
types: [labeled] | ||
push: | ||
branches: | ||
- master | ||
|
||
jobs: | ||
test: | ||
|
||
strategy: | ||
fail-fast: false # don't cancel other matrix jobs when one fails | ||
matrix: | ||
python-version: [3.6, 3.7, 3.8, 3.9] | ||
kandersolar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
suffix: [''] # the alternative to "-min" | ||
include: | ||
- python-version: 3.6 | ||
suffix: -min | ||
|
||
runs-on: ubuntu-latest | ||
if: (github.event_name == 'pull_request_target' && contains(github.event.pull_request.labels.*.name, 'remote-data')) || (github.event_name == 'push') | ||
|
||
steps: | ||
- uses: actions/checkout@v3 | ||
if: github.event_name == 'pull_request_target' | ||
# pull_request_target runs in the context of the target branch (pvlib/master), | ||
# but what we need is the hypothetical merge commit from the PR: | ||
with: | ||
ref: "refs/pull/${{ github.event.number }}/merge" | ||
|
||
- uses: actions/checkout@v2 | ||
if: github.event_name == 'push' | ||
|
||
- name: Set up conda environment | ||
uses: conda-incubator/setup-miniconda@v2 | ||
with: | ||
activate-environment: test_env | ||
environment-file: ${{ env.REQUIREMENTS }} | ||
python-version: ${{ matrix.python-version }} | ||
auto-activate-base: false | ||
env: | ||
# build requirement filename. First replacement is for the python | ||
# version, second is to add "-min" if needed | ||
REQUIREMENTS: ci/requirements-py${{ matrix.python-version }}${{ matrix.suffix }}.yml | ||
|
||
- name: List installed package versions | ||
shell: bash -l {0} # necessary for conda env to be active | ||
run: conda list | ||
|
||
- name: Run tests | ||
shell: bash -l {0} # necessary for conda env to be active | ||
env: | ||
# copy GitHub Secrets into environment variables for the tests to access | ||
NREL_API_KEY: ${{ secrets.NRELAPIKEY }} | ||
BSRN_FTP_USERNAME: ${{ secrets.BSRN_FTP_USERNAME }} | ||
BSRN_FTP_PASSWORD: ${{ secrets.BSRN_FTP_PASSWORD }} | ||
run: pytest pvlib/tests/iotools pvlib/tests/test_forecast.py --cov=./ --cov-report=xml --remote-data | ||
|
||
- name: Upload coverage to Codecov | ||
if: matrix.python-version == 3.6 && matrix.suffix == '' | ||
uses: codecov/codecov-action@v2 | ||
with: | ||
fail_ci_if_error: true | ||
verbose: true | ||
flags: remote-data # flags are configured in codecov.yml |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
name: pytest | ||
|
||
on: | ||
pull_request: | ||
push: | ||
branches: | ||
- master | ||
|
||
jobs: | ||
test: | ||
|
||
strategy: | ||
fail-fast: false # don't cancel other matrix jobs when one fails | ||
matrix: | ||
# use macos-10.15 instead of macos-latest for py3.6 support | ||
os: [ubuntu-latest, macos-10.15, windows-latest] | ||
python-version: [3.6, 3.7, 3.8, 3.9] | ||
environment-type: [conda, bare] | ||
suffix: [''] # placeholder as an alternative to "-min" | ||
include: | ||
- os: ubuntu-latest | ||
python-version: 3.6 | ||
environment-type: conda | ||
suffix: -min | ||
exclude: | ||
- os: macos-10.15 | ||
environment-type: conda | ||
- os: windows-latest | ||
environment-type: bare | ||
|
||
runs-on: ${{ matrix.os }} | ||
|
||
steps: | ||
- uses: actions/checkout@v3 | ||
|
||
- name: Set up conda environment | ||
if: matrix.environment-type == 'conda' | ||
uses: conda-incubator/setup-miniconda@v2 | ||
with: | ||
activate-environment: test_env | ||
environment-file: ${{ env.REQUIREMENTS }} | ||
python-version: ${{ matrix.python-version }} | ||
auto-activate-base: false | ||
env: | ||
# build requirement filename. First replacement is for the python | ||
# version, second is to add "-min" if needed | ||
REQUIREMENTS: ci/requirements-py${{ matrix.python-version }}${{ matrix.suffix }}.yml | ||
|
||
- name: List installed package versions (conda) | ||
if: matrix.environment-type == 'conda' | ||
shell: bash -l {0} # necessary for conda env to be active | ||
run: conda list | ||
|
||
- name: Install bare Python ${{ matrix.python-version }}${{ matrix.suffix }} | ||
if: matrix.environment-type == 'bare' | ||
uses: actions/setup-python@v1 | ||
with: | ||
python-version: ${{ matrix.python-version }} | ||
|
||
- name: Set up bare environment | ||
if: matrix.environment-type == 'bare' | ||
run: | | ||
pip install .[test] | ||
pip freeze | ||
|
||
- name: Run tests | ||
shell: bash -l {0} # necessary for conda env to be active | ||
run: | | ||
# ignore iotools & forecast; those tests are run in a separate workflow | ||
pytest pvlib --cov=./ --cov-report=xml --ignore=pvlib/tests/iotools --ignore=pvlib/tests/test_forecast.py | ||
|
||
- name: Upload coverage to Codecov | ||
if: matrix.python-version == 3.6 && matrix.suffix == '' && matrix.os == 'ubuntu-latest' && matrix.environment-type == 'conda' | ||
uses: codecov/codecov-action@v2 | ||
with: | ||
fail_ci_if_error: true | ||
verbose: true | ||
flags: core # flags are configured in codecov.yml |
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kanderso-nrel if we were to delete this line, then we wouldn't have to add and remove the remote-data label every time to trigger the tests.
For PRs that do NOT have the remote-data label, the remote tests would simply be skipped due to the if statement in line 66:

Maybe there are some security aspects that I'm not aware of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having thought about this more, I think this was intentional on my part (see the comments at the top of the workflow file). Removing this line would allow a scenario where someone can submit a benign PR to lure us into adding the
remote-data
label, then push another commit with malicious content and have the workflow run without our sign off. The way it is now, the workflow cannot run without a maintainer explicitly triggering it.Whether keeping that security hole plugged is worth the annoyance of repeated labeling is another question...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyPI's recent labeling of pvlib as a critical project has me feeling like we should err on the side of caution here. Tests should have a minimum of real API calls anyways - the vast majority should be mocked.