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

fix: add missing noop param for full-sync #733

Open
wants to merge 1 commit into
base: main-enterprise
Choose a base branch
from

Conversation

PendaGTP
Copy link
Contributor

@PendaGTP PendaGTP commented Jan 7, 2025

Related to #378 (#378 (comment))

Now the full-sync script will use the new env var FULL_SYNC_NOOP and so run is noop mode if true.
This helps run safe-settings GHA since you can add this var conditionaly based on event.

Example (based from https://github.com/UCL-MIRSG/.github/blob/4695e545829b91dcddc6e36358454bc4a879f751/.github/workflows/safe-settings.yaml thanks @paddyroddy) :

name: Safe Settings Sync
on:
  push:
    branches:
      - main
  pull_request:
    paths:
      - safe-settings/**
      - .github/workflows/safe-settings.yaml
  schedule:
    - cron: 0 */4 * * *
  # -->
  workflow_dispatch:
    inputs:
      NOOP:
        description: 'Run in no-op mode'
        required: false
        type: boolean
        default: false

concurrency:
  cancel-in-progress: true
  group: >-
    ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}

jobs:
  safe-settings-sync:
    runs-on: ubuntu-latest
    env:
      SAFE_SETTINGS_VERSION: 2.1.14
      SAFE_SETTINGS_CODE_DIR: .safe-settings-code
    steps:
      - name: Checkout source
        uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4

      - name: Checkout GitHub Safe-Settings repository
        uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4
        with:
          path: ${{ env.SAFE_SETTINGS_CODE_DIR }}
          ref: ${{ env.SAFE_SETTINGS_VERSION }}
          repository: github/safe-settings

      - name: Setup Node.js
        uses: actions/setup-node@39370e3970a6d050c480ffad4ff0ed4d3fdee5af # v4
        with:
          cache-dependency-path:
            ${{ env.SAFE_SETTINGS_CODE_DIR }}/package-lock.json
          cache: npm
          node-version-file: ${{ env.SAFE_SETTINGS_CODE_DIR }}/.nvmrc

      - name: Install dependencies
        run: npm install
        working-directory: ${{ env.SAFE_SETTINGS_CODE_DIR }}

      # -->
      - name: Set no-op mode flag
        run: |
          echo "FULL_SYNC_NOOP=false" >> $GITHUB_ENV

          if [[ "${{ github.event_name }}" == "pull_request" ]]; then
            echo "FULL_SYNC_NOOP=true" >> $GITHUB_ENV
          fi

          if [[ "${{ github.event_name }}" == "workflow_dispatch" ]]; then
            if [[ "${{ inputs.NOOP }}" != "" ]]; then
              echo "FULL_SYNC_NOOP=${{ inputs.NOOP }}" >> $GITHUB_ENV
            fi
          fi

      - name: Run application
        run: npm run full-sync
        working-directory: ${{ env.SAFE_SETTINGS_CODE_DIR }}
        env:
          ADMIN_REPO: .github
          APP_ID: ${{ vars.SAFE_SETTINGS_APP_ID }}
          BLOCK_REPO_RENAME_BY_HUMAN: false
          CONFIG_PATH: safe-settings
          DEPLOYMENT_CONFIG_FILE:
            ${{ github.workspace }}/safe-settings/deployment.yaml
          ENABLE_PR_COMMENT: true
          GH_ORG: ${{ vars.SAFE_SETTINGS_GH_ORG }}
          GITHUB_CLIENT_ID: ${{ vars.SAFE_SETTINGS_GITHUB_CLIENT_ID }}
          GITHUB_CLIENT_SECRET:
            ${{ secrets.SAFE_SETTINGS_GITHUB_CLIENT_SECRET }}
          LOG_LEVEL: trace
          PRIVATE_KEY: ${{ secrets.SAFE_SETTINGS_PRIVATE_KEY }}
          SETTINGS_FILE_PATH: organisation.yaml
          # -->
          FULL_SYNC_NOOP: ${{ env.FULL_SYNC_NOOP }}

@HagegeR
Copy link

HagegeR commented Jan 8, 2025

Nice! maybe you can update the readme too?
https://github.com/github/safe-settings/blob/main-enterprise/docs/github-action.md

@PendaGTP
Copy link
Contributor Author

PendaGTP commented Jan 8, 2025

Nice! maybe you can update the readme too? https://github.com/github/safe-settings/blob/main-enterprise/docs/github-action.md

Yes, I agree that updating the documentation would be beneficial.

I think it would be more appropriate to create a separate PR for GHA documentation updates, as I'm planning to make other documentation improvements unrelated to this PR.

But if updating GHA documentation in this current PR is required to merge it, I can update it.

@PendaGTP PendaGTP force-pushed the fix/full-sync-with-noop branch from fd14365 to ef70108 Compare January 8, 2025 12:10
@@ -5,5 +5,6 @@ module.exports = {
DEPLOYMENT_CONFIG_FILE: process.env.DEPLOYMENT_CONFIG_FILE || 'deployment-settings.yml',
CREATE_PR_COMMENT: process.env.CREATE_PR_COMMENT || 'true',
CREATE_ERROR_ISSUE: process.env.CREATE_ERROR_ISSUE || 'true',
BLOCK_REPO_RENAME_BY_HUMAN: process.env.BLOCK_REPO_RENAME_BY_HUMAN || 'false'
BLOCK_REPO_RENAME_BY_HUMAN: process.env.BLOCK_REPO_RENAME_BY_HUMAN || 'false',
FULL_SYNC_NOOP: process.env.FULL_SYNC_NOOP === 'true'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@decyjphr

We have different implementations for boolean environment variables.
For example, CREATE_PR_COMMENT is handled as a string, while with this changes FULL_SYNC_NOOP is implemented as a boolean value. This is reflected in the different approaches in our unit tests.

I think it would be beneficial to standardize how we handle boolean environment variables across the project. Once we decide on the preferred implementation (string or boolean), I can create a new PR to refactor the existing code for consistency.
I can also update my changes to fit current implementation.

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.

2 participants