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

PRs from forks break #44

Open
liamhuber opened this issue Jun 5, 2023 · 1 comment
Open

PRs from forks break #44

liamhuber opened this issue Jun 5, 2023 · 1 comment
Assignees

Comments

@liamhuber
Copy link
Member

After applying the centralized CI to pyiron_contrib, the dependabot PRs revealed that the current automated update to docs and binder environment files breaks for third-party PRs, e.g.:

Run actions/checkout@v3
  with:
    ref: dependabot/pip/scikit-image-0.21.0
    fetch-depth: 0
    repository: pyiron/pyiron_contrib
    ssh-strict: true
    persist-credentials: true
    clean: true
    lfs: false
    submodules: false
    set-safe-directory: true
Error: Input required and not supplied: token

Going back and re-reading the security docs confirmed our suspicion that the secrets are not getting passed nor is write permission available from external forks using on: pull_request. We do get these powers in the dependabot PR because that is explicitly on: pull_request_target.

So before pyiron_contrib was on the centralized CI, the only place that got third-party write access and secrets was the dependabot workflow. Now, under the centralized CI we are expecting these privileges in the main on: pull_request workflow in order to keep the docs/environment.yml and .binder/environment.yml files synchronized with .ci_support/environment.yml (plus the extra stuff we need for docs/notebooks).

The advantage to the centralized CI way is that we never need to modify the docs and binder environments, they are just always kept up-to-date.

One solution:

  • Add this environment update to the dependabot update on: pull_request_target script
    • With the same actor filtering we have for the existing environment update providing security
    • Now dependabot will propagate its environment updates through to the other env files
  • Filter this environment update step to only happen on PRs originating from pyiron
    • Non-dependabot forks may possibly result in the docs and binder envs getting out-of-date
      • This will be fixed automatically on the next PR from either pyiron or dependabot
    • Dependabot will not be double-propagating the changes, since it gets filtered out here
  • Remove the needs: commit-updated-env from the rest of the CI steps in the main push-pull workflow
    • These were here for efficiency, since the other steps would sometimes start, then get killed and restarted when there was a commit to update the envs, so we're very slightly wasteful by removing this.

Then dependabot can update all the env files it needs to, the dependent env-files stay (almost always) up-to-date, and the rest of the CI is free to run even on third-party PRs.

If you like it, I'll take care of implementation, but I wanted to solicit feedback on the plan first.

@liamhuber
Copy link
Member Author

liamhuber commented Nov 22, 2023

Just coming back to this since forever and trying to re-understand it and figure out where we are

  • Add this environment update to the dependabot update
  • Filter this environment update step to only happen on PRs originating from pyiron
  • Remove the needs: commit-updated-env from the rest of the CI steps in the main push-pull workflow
    • Easy, just edits to the reusable workflow

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

No branches or pull requests

3 participants