Can't get PR evaluation to run in dry-run mode (run via GitHub actions)

[sumokadet]

Problem Description

We can't get safe-settings to run in dry-run (nop?) mode when evaluating pull requests on topic branch, run via GitHub actions. It always enforces the rules for both github triggers pull_request and push from main branch even though on topic branch.

Are we missing a setting?

We are on rc version 2.1.15-rc.1, feel free to discard this issue if we are reporting it premature. We are ready to wait for future versions, I just wanted to report this back. Great work by the way! 👍

What is actually happening

We see that the settings evaluated are not from the topic branch (they are from main branch) and that the debug statements display that it is not run in nop mode. Any changes are enforced from main branch even though it's evaluation intended.

It's as if it doesn't recognize that it should be run in dry-run (nop) mode and picks up the topic branch name.

What is the expected behavior

That when run with event trigger is pull_request that the settings evaluated will be from the topic branch (not main branch) and that the changes on topic branch ARE NOT enforced, just gets output on the PR.

Error output, if available

The log from full-sync.js when evaluating a pull request with LOG_LEVEL: debug displays that

a) The GET statement doesn't invoke the GitHub api for topic branch, but just main branch. I would expect that it tried to look the file from ?ref=topic-branch or something like that

DEBUG (github): GitHub request: GET https://api.github.com/repos/OurOrg/our-repo/contents/safe-settings%2Frepos%test-repo.yml - 200

b) The output from probat displays it's not run in nop:

DEBUG (probot): Not run in nop

Context

Running on GitHub Enterprise. Running safe-settings via GitHub Action. Run with pull_request trigger.

on:
  # PR - Pull request trigger
  pull_request:

Run with settings mentioned in example file. Almost as example. The admin repo has a custom name.

    SAFE_SETTINGS_VERSION: 2.1.15-rc.1

...

      - run: npm run full-sync
        working-directory: ${{env.SAFE_SETTINGS_CODE_DIR}}
        env:
          # Configuration files placed in safe-settings-folder in the ADMIN_REPO
          ADMIN_REPO: our-admin-repo
          CONFIG_PATH: safe-settings
          SETTINGS_PATH: ${{github.workspace}}/safe-settings/settings.yml
          DEPLOYMENT_CONFIG_FILE: ${{github.workspace}}/safe-settings/deployment-settings.yml

          # GitHub App settings
          GH_ORG: ${{vars.SAFE_SETTINGS_GH_ORG}}
          APP_ID: ${{vars.SAFE_SETTINGS_APP_ID}}
          PRIVATE_KEY: ${{secrets.SAFE_SETTINGS_PRIVATE_KEY}}
          GITHUB_CLIENT_ID: ${{vars.SAFE_SETTINGS_GITHUB_CLIENT_ID}}
          GITHUB_CLIENT_SECRET: ${{secrets.SAFE_SETTINGS_GITHUB_CLIENT_SECRET}}
          
          # Logging
          LOG_LEVEL: debug # info, trace, debug

[sumokadet]

I can't share own run since it's in a private organization, but it's identical to what we see here (PR evaluation on branch), 2.1.14:

• https://github.com/UCL-MIRSG/.github/actions/runs/12585780658/job/35078280748?pr=153#step:6:90
  (the call is without ref=branch param, hence the lookup in on main branch)
• https://github.com/UCL-MIRSG/.github/actions/runs/12585780658/job/35078280748?pr=153#step:6:16806
  (not run in nop)

[PendaGTP]

@sumokadet

Regarding the issue where safe-settings doesn't execute in nop (dry-run) mode (when running full-sync), you can track the progress through these links:

• Related issue: Custom properties are applied during pull request #734
• PR:
  • fix: add missing nop logic for custom properties plugin #738
  • fix: add missing nop param for full-sync #733

[PendaGTP]

@decyjphr - Regarding the issue with the full-sync script always evaluating settings from the default branch:

I'd be happy to contribute to fixing this. We can we start by creating a dedicated issue to discuss the implementation details, since we currently can't retrieve the context in the same way as with event handlers.
My initial proposal would be to add a new environment variable called FULL_SYNC_FROM_REF (defaulting to the default branch). This could be easily configured in CI environments.

@sumokadet - Would adding an environment variable to specify which ref to use for settings meet your needs?

Note: This change would also require addressing the nop mode in full-sync (related to #733).

Please let me know your thoughts on this approach.

P.S. @decyjphr - I realize I might be tagging you too frequently. Please let me know if this is bothersome - I'm still learning how best to collaborate in open source and want to be respectful of everyone's notifications.

[sumokadet]

@PendaGTP - Thanks and kudos for quick response! The combination of input env vars FULL_SYNC_NOOP (dry-run) (from #733) and FULL_SYNC_FROM_REF (or named FULL_SYNC_GIT_REF) (which ref to get settings from) will be good for our needs.

From a "user perspective" in a workflow it could probably look a little bit like this. It would be nice if FULL_SYNC_FROM_REF defaults to same value as ${{ github.ref }} so it both works when the var is mentioned and when it is not provided (should be default branch).

        env:
          ...
          FULL_SYNC_NOOP: ${{ github.event_name == 'pull_request' && 'true' || 'false' }} # Or just "real" bool type?
          FULL_SYNC_FROM_REF: ${{ github.ref }}

[PendaGTP]

@sumokadet

Thanks for your feedback!

I think one thing that could also help from a user perspective would be to create a GitHub Action. This way, the user would only need to provide the necessary inputs and the action would take care of the rest logic/setup.

Any thoughts on this approach? I'd be happy to create an issue to discuss this further and potentially submit a PR with a GHA implementation if this seems like a helpful direction to pursue.

[decyjphr]

@sumokadet @PendaGTP Another way we could make this work is using the feature in probot to simulate the app getting a webhook event.

So this would be basically having a GHA in the admin repo which will listen to push and and PR events and then call the app simulating the webhook and the payload.

[PendaGTP]

@decyjphr Thank you for your feedback. This approach could make the handling more consistent with our event processing. I'll review our current logic around nop, checks, and PR change previews to better understand the current workflow.

[sumokadet]

@PendaGTP regarding "Creating a wrapper action" #739 (comment)

Good idea with a separate issue. An action would be nice in the future.

However, I'm a bit in doubt about if the users of safe-settings (which I believe are super-users/administrative types) will go to the action first. I assume they need to deep dive into how this safe-settings works since it potentially alters all repos in their organization, hence they maybe want to alter how it's called. Anyways, recommendations in documentation for this should be added.

[powerman]

However, I'm a bit in doubt about if the users of safe-settings (which I believe are super-users/administrative types) will go to the action first.

No matter what we are using at work, we also have personal repos!
I've own server and run a dozen of docker containers there with different services, but even to me it feels too annoying to add another one with safe-settings just to setup settings for some personal repos in manageable way.
So, using safe-settings as an action looks like the best of available possibilities for quick personal use.
Absent support for webhooks is okay, but missing PR checks/comments/dry-run feature is not so good.