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

Safely run reviewdog on pull_request_target events #1427

Merged
merged 4 commits into from
Apr 17, 2024

Conversation

jcapriot
Copy link
Member

Summary

Updates permissions for reviewdog actions.

Looks like it explicitly needs to be given read permissions for the contents, and write permissions for the pull-requests to modify PR's from public forks.

Copy link

codecov bot commented Apr 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.54%. Comparing base (999d18c) to head (d6c911d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1427   +/-   ##
=======================================
  Coverage   82.54%   82.54%           
=======================================
  Files         167      167           
  Lines       25700    25700           
=======================================
  Hits        21214    21214           
  Misses       4486     4486           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@santisoler
Copy link
Member

I'm not sure I understood why it needs writing permissions. Does it push changes running black on the PRs? Or it just comments suggestions on the PRs?

@jcapriot
Copy link
Member Author

jcapriot commented Apr 16, 2024

It needs write access to comment on the PR's, but apparently it doesn't get that permission from PR's opened from forked repositories... Whereas the first PR I made had to come from this repo itself in order to run the first action, it went unnoticed. I'm still looking at options to set though.

@jcapriot
Copy link
Member Author

Ok, just figured this out in what is probably the safest way to do this. First though the issue:

For workflows triggered from pull requests from forks of public repositories, secrets.GITHUB_TOKEN only has read permissions, and this cannot be changed by anyone.

  • This is a good thing: you don't want to open up the repository to arbitrary code execution that could modify the repository from outside actors (even if we still have to approve workflows to run from first-time contributors).

The solution:
For every pull request, two GitHub events are triggered, pull_request and pull_request_target. The pull_request_target event runs are in the context of the target of a pull request, therefore it runs with all of the permissions associated with a branch on this repository, meaning secret.GITHUB_TOKEN gets read and write access. Note:

  • You still don't want to execute anything that depends on items from the pull_request's source. (i.e. don't run pip install -r requirements.txt when requirements.txt would be from the pr source.) That would defeat the purpose of holding write permissions from the public PRs.

So what this modification here does for a pull request is:
For each pull request:

  • Trigger the pr target's workflow (not the pr source's workflow).
    • Download's the target branch source code
    • Install's style requirements defined in the target's source code
    • Download's the pull request source into another working directory
    • runs reviewdog in the pull request's working directory (This step only analyzes the code, it will never execute anything in that source.) The only thing it will look at that effects the execution of the style checkers are black and flake8 configurations (which again just parse those as text files).

@jcapriot jcapriot changed the title add permissions block for reviewdog Safely run reviewdog on pull_request_target events Apr 17, 2024
@jcapriot
Copy link
Member Author

Note that when we update the flake8 or black versions in the requirements file, this will not alter the annotation workflow until it is merged into the target, so there might be an issue causing out-of-sync comments, but it would only effect that specific PR, and be resolved once it's merged.

@jcapriot jcapriot merged commit 8a763d3 into simpeg:main Apr 17, 2024
16 checks passed
@jcapriot jcapriot deleted the reviewdog_permissions branch April 17, 2024 16:40
thibaut-kobold added a commit to KoBoldMetals/simpeg that referenced this pull request May 8, 2024
…e_simpeg_021

* commit 'fe444e2d714ab1a3f1541575a9f8c4ad1e74894b': (73 commits)
  Fix distance calculation in `convert_survey_3d_to_2d_lines` (simpeg#1390)
  Rename spontaneous potential to self potential (simpeg#1422)
  Lowercase simpeg for generating coverage on Azure (simpeg#1443)
  Replace SimPEG for simpeg across docstrings (simpeg#1438)
  Update copyright year in __init__.py (simpeg#1436)
  Move to a PEP8 compliant package name. (simpeg#1430)
  Fix rst syntax in release notes for v0.21.0 (simpeg#1434)
  Replace use of `refine_tree_xyz` in DCIP tutorials (simpeg#1381)
  Add new Issue template for making a release (simpeg#1410)
  Safely run reviewdog on `pull_request_target` events (simpeg#1427)
  Use reviewdog to annotate PR's with black and flake8 errors. (simpeg#1424)
  Remove the parameters argument from docstring (simpeg#1417)
  Add release notes for v0.21.1 (simpeg#1416)
  Fix hard dask dependency (simpeg#1415)
  Publish documentation on azure (simpeg#1412)
  Add release notes for SimPEG v0.21 (simpeg#1409)
  Bump Black version to 24.3.0 (simpeg#1403)
  Remove link to "twitter" (simpeg#1406)
  Add Ying and Williams to AUTHORS.rst (simpeg#1405)
  Dask MetaSim (simpeg#1199)
  ...

# Conflicts:
#	simpeg/electromagnetics/base_1d_stitched.py
#	simpeg/electromagnetics/frequency_domain/simulation_1d_stitched.py
#	simpeg/electromagnetics/time_domain/receivers.py
#	simpeg/electromagnetics/time_domain/simulation_1d_stitched.py
#	simpeg/electromagnetics/utils/em1d_utils.py
#	simpeg/potential_fields/magnetics/_numba_functions.py
#	simpeg/regularization/laterally_constrained.py
#	simpeg/regularization/regularization_mesh_lateral.py
#	simpeg/regularization/rotated.py
#	simpeg/utils/__init__.py
#	simpeg/utils/model_utils.py
#	tests/base/regularizations/test_full_gradient.py
#	tests/base/regularizations/test_regularization.py
#	tests/base/test_mass_matrices.py
#	tests/base/test_model_utils.py
#	tests/base/test_utils.py
#	tests/em/fdem/inverse/adjoint/test_FDEM_adjointEB.py
#	tests/em/fdem/inverse/derivs/test_FDEM_derivs.py
#	tests/pf/test_forward_Mag_Linear.py
thibaut-kobold added a commit to KoBoldMetals/simpeg that referenced this pull request May 8, 2024
…al_update_21_release

* commit '07aeb65de865cd9df55e2d51ca840ea9d60f2234': (67 commits)
  Fix rst syntax in release notes for v0.21.0 (simpeg#1434)
  Replace use of `refine_tree_xyz` in DCIP tutorials (simpeg#1381)
  Add new Issue template for making a release (simpeg#1410)
  Safely run reviewdog on `pull_request_target` events (simpeg#1427)
  Use reviewdog to annotate PR's with black and flake8 errors. (simpeg#1424)
  Remove the parameters argument from docstring (simpeg#1417)
  Add release notes for v0.21.1 (simpeg#1416)
  Fix hard dask dependency (simpeg#1415)
  Publish documentation on azure (simpeg#1412)
  Add release notes for SimPEG v0.21 (simpeg#1409)
  Bump Black version to 24.3.0 (simpeg#1403)
  Remove link to "twitter" (simpeg#1406)
  Add Ying and Williams to AUTHORS.rst (simpeg#1405)
  Dask MetaSim (simpeg#1199)
  Update year in LICENSE (simpeg#1404)
  Update AUTHORS.rst (simpeg#1259)
  Minor adjustments to Sphinx configuration (simpeg#1398)
  Enforce regularization `weights` as dictionaries (simpeg#1344)
  Improve documentation for base simulation classes (simpeg#1295)
  Add link to User Tutorials to navbar in docs (simpeg#1401)
  ...

# Conflicts:
#	SimPEG/electromagnetics/time_domain/receivers.py
#	SimPEG/utils/__init__.py
#	SimPEG/utils/model_utils.py
#	tests/base/regularizations/test_full_gradient.py
#	tests/base/regularizations/test_regularization.py
#	tests/base/test_model_utils.py
#	tests/base/test_utils.py
#	tests/pf/test_forward_Mag_Linear.py
thibaut-kobold added a commit to thibaut-kobold/simpeg that referenced this pull request May 9, 2024
…ting_strategy

* commit 'fe444e2d714ab1a3f1541575a9f8c4ad1e74894b': (86 commits)
  Fix distance calculation in `convert_survey_3d_to_2d_lines` (simpeg#1390)
  Rename spontaneous potential to self potential (simpeg#1422)
  Lowercase simpeg for generating coverage on Azure (simpeg#1443)
  Replace SimPEG for simpeg across docstrings (simpeg#1438)
  Update copyright year in __init__.py (simpeg#1436)
  Move to a PEP8 compliant package name. (simpeg#1430)
  Fix rst syntax in release notes for v0.21.0 (simpeg#1434)
  Replace use of `refine_tree_xyz` in DCIP tutorials (simpeg#1381)
  Add new Issue template for making a release (simpeg#1410)
  Safely run reviewdog on `pull_request_target` events (simpeg#1427)
  Use reviewdog to annotate PR's with black and flake8 errors. (simpeg#1424)
  Remove the parameters argument from docstring (simpeg#1417)
  Add release notes for v0.21.1 (simpeg#1416)
  Fix hard dask dependency (simpeg#1415)
  Publish documentation on azure (simpeg#1412)
  Add release notes for SimPEG v0.21 (simpeg#1409)
  Bump Black version to 24.3.0 (simpeg#1403)
  Remove link to "twitter" (simpeg#1406)
  Add Ying and Williams to AUTHORS.rst (simpeg#1405)
  Dask MetaSim (simpeg#1199)
  ...

# Conflicts:
#	simpeg/utils/__init__.py
#	simpeg/utils/model_utils.py
#	tests/base/test_model_utils.py
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.

None yet

2 participants