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

Config merge not working as expected for settings files #723

Open
riprasad opened this issue Dec 30, 2024 · 0 comments
Open

Config merge not working as expected for settings files #723

riprasad opened this issue Dec 30, 2024 · 0 comments
Labels
bug Something isn't working

Comments

@riprasad
Copy link

riprasad commented Dec 30, 2024

Problem Description

As stated in the README

The precedence order for settings file is repository > suborg > org (.github/repos/.yml > .github/suborgs/.yml > .github/settings.yml)

This implies that if the same configuration(s) are defined at different levels, the settings defined at the repository level should take precedence. However, this is not happening as expected. I discovered that the issue stems from the way safe-settings creates the final merged config from the settings files. The merge process does a union of the suborg and repo level settings file, which brings back the deleted configs and also creates duplicate elements. There's also an edge case where the API call fails due to duplicate elements in the merged config.

What is actually happening

I have configured a ruleset at the suborganization level that applies to multiple repositories and then modify the same ruleset for a particular repository in the repo level settings file.

suborg/base-rulesets.yml

suborgrepos:
  - '*'

rulesets:
  - name: Base Rulesets
    target: branch
    enforcement: active
    bypass_actors:
      - actor_id: 1
        actor_type: OrganizationAdmin
        bypass_mode: always

      - actor_id: 1097369
        actor_type: Integration
        bypass_mode: pull_request

      - actor_id: 11825178
        actor_type: Team
        bypass_mode: pull_request

    conditions:
      ref_name:
        include:
          - "refs/heads/rhoai-*"
        exclude: []

    rules:
      - type: deletion

      - type: non_fast_forward

      - type: pull_request
        parameters:
          required_approving_review_count: 0
          dismiss_stale_reviews_on_push: false
          require_code_owner_review: false
          require_last_push_approval: false
          required_review_thread_resolution: false
          allowed_merge_methods:
            - merge
            - squash
            - rebase

repos/repo-name.yml

rulesets:
  - name: Base Rulesets
    target: branch
    enforcement: active
    bypass_actors:
      - actor_id: 11825178
        actor_type: Team
        bypass_mode: pull_request

    conditions:
      ref_name:
        include:
          - "refs/heads/rhoai-*"
        exclude: []

    rules:
      - type: creation

      - type: non_fast_forward

      - type: pull_request
        parameters:
          required_approving_review_count: 1
          dismiss_stale_reviews_on_push: true
          require_code_owner_review: false
          require_last_push_approval: false
          required_review_thread_resolution: false
          allowed_merge_methods:
            - merge
            - squash
            - rebase

What's happening is that the configurations from the suborgs and repo level settings files are getting merged as shown below. The merge process does a union of the suborg and repo level settings file, which brings back the deleted configs and also creates duplicate elements.

target: "branch"
    enforcement: "active"
    bypass_actors: [
      {
        "actor_id": 1,
        "actor_type": "OrganizationAdmin",
        "bypass_mode": "always"
      },
      {
        "actor_id": 1097369,
        "actor_type": "Integration",
        "bypass_mode": "pull_request"
      },
      {
        "actor_id": 11825178,
        "actor_type": "Team",
        "bypass_mode": "pull_request"
      },
      {
        "actor_id": 11825178,
        "actor_type": "Team",
        "bypass_mode": "pull_request"
      }
    ]
    conditions: {
      "ref_name": {
        "include": [
          "refs/heads/rhoai-*",
          "refs/heads/rhoai-*"
        ],
        "exclude": []
      }
    }
    rules: [
      {
        "type": "deletion"
      },
      {
        "type": "non_fast_forward"
      },
      {
        "type": "pull_request",
        "parameters": {
          "required_approving_review_count": 0,
          "dismiss_stale_reviews_on_push": false,
          "require_code_owner_review": false,
          "require_last_push_approval": false,
          "required_review_thread_resolution": false,
          "allowed_merge_methods": [
            "merge",
            "squash",
            "rebase"
          ]
        }
      },
      {
        "type": "creation"
      },
      {
        "type": "non_fast_forward"
      },
      {
        "type": "pull_request",
        "parameters": {
          "required_approving_review_count": 1,
          "dismiss_stale_reviews_on_push": true,
          "require_code_owner_review": false,
          "require_last_push_approval": false,
          "required_review_thread_resolution": false,
          "allowed_merge_methods": [
            "merge",
            "squash",
            "rebase"
          ]
        }
      }
    ]

In the repository-level settings file, I made the below modifications to the rules, but few of them were not applied as expected:

  • removed deletion rule - This rule was brought back due to the merge
  • added creation rule Works as expected
  • changed required_approving_review_count from 0 to 1 - Works as expected
  • changed dismiss_stale_reviews_on_push from false to true - Works as expected
  • removed actor_id: 1 and actor_id: 1097369 from bypass_actors list - These actors were brought back due to merge.


Apart from this, the merge is also creating DUPLICATE PROPERTIES, which is resulting in error

  • If the ruleset exists, the app successfully sends a PUT request with the merged config as the request body(full logs). However, when the ruleset doesn't exist, the app makes a POST request with the same merged config, which leads to the below error(full logs).

ERROR (HttpError): invalid json response body at https://api.github.com/repos/rhoai-rhtap/rhods-operator/rulesets reason: Unexpected end of JSON input
HttpError: invalid json response body at https://api.github.com/repos/rhoai-rhtap/rhods-operator/rulesets reason: Unexpected end of JSON input


- include": ["refs/heads/rhoai-*","refs/heads/rhoai-*"] is causing Screenshot 2024-12-30 at 4 26 21 PM.

What is the expected behavior

The Rulesets created for the repository should exactly match the configuration defined in the repository-level settings file.

  • Rules removed from the repository-level settings file should not be included in the ruleset.
  • Actor IDs removed from the repository-level settings file should not appear in the bypass actors.

In general - the merged config should not have duplicate elements and should not bring back deleted configurations in the repo level settings file. Safe-settings should consider the repository-level settings file as the source of truth and ensure that the precedence order is respected. Hopefully, it will also prevents API calls from failing due to duplicate elements in the request body.

Error output, if available

Full Logs - https://github.com/rhoai-rhtap/admin/actions/runs/12546352801/job/34982053584

ERROR (HttpError): invalid json response body at https://api.github.com/repos/rhoai-rhtap/rhods-operator/rulesets reason: Unexpected end of JSON input
    HttpError: invalid json response body at https://api.github.com/repos/rhoai-rhtap/rhods-operator/rulesets reason: Unexpected end of JSON input
        at /home/runner/work/admin/admin/.safe-settings-code/node_modules/probot/node_modules/@octokit/request/dist-node/index.js:108:11
        at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
        at async sendRequestWithRetries (/home/runner/work/admin/admin/.safe-settings-code/node_modules/octokit-auth-probot/node_modules/@octokit/auth-app/dist-node/index.js:398:12)
        at async Job.doExecute (/home/runner/work/admin/admin/.safe-settings-code/node_modules/bottleneck/light.js:405:18)
status: 500
request: {
    method: "POST"
    url: "https://api.github.com/repos/rhoai-rhtap/rhods-operator/rulesets"
    headers: {
      "accept": "application/vnd.github.v3+json",
      "user-agent": "probot/12.3.4 octokit-core.js/3.6.0 Node.js/20.18.1 (linux; x64)",
      "x-github-api-version": "2022-11-28",
      "authorization": "token [REDACTED]",
      "content-type": "application/json; charset=utf-8"
    }
    body: "{\"name\":\"Base Rulesets\",\"target\":\"branch\",\"enforcement\":\"active\",\"bypass_actors\":[{\"actor_id\":1,\"actor_type\":\"OrganizationAdmin\",\"bypass_mode\":\"always\"},{\"actor_id\":1097369,\"actor_type\":\"Integration\",\"bypass_mode\":\"pull_request\"},{\"actor_id\":11825178,\"actor_type\":\"Team\",\"bypass_mode\":\"pull_request\"},{\"actor_id\":11825178,\"actor_type\":\"Team\",\"bypass_mode\":\"pull_request\"}],\"conditions\":{\"ref_name\":{\"include\":[\"refs/heads/rhoai-*\",\"refs/heads/rhoai-*\"],\"exclude\":[]}},\"rules\":[{\"type\":\"deletion\"},{\"type\":\"non_fast_forward\"},{\"type\":\"pull_request\",\"parameters\":{\"required_approving_review_count\":0,\"dismiss_stale_reviews_on_push\":false,\"require_code_owner_review\":false,\"require_last_push_approval\":false,\"required_review_thread_resolution\":false,\"allowed_merge_methods\":[\"merge\",\"squash\",\"rebase\"]}},{\"type\":\"creation\"},{\"type\":\"non_fast_forward\"},{\"type\":\"pull_request\",\"parameters\":{\"required_approving_review_count\":1,\"dismiss_stale_reviews_on_push\":true,\"require_code_owner_review\":false,\"require_last_push_approval\":false,\"required_review_thread_resolution\":false,\"allowed_merge_methods\":[\"merge\",\"squash\",\"rebase\"]}}]}"
    request: {
      "retryCount": 3,
      "retries": 3,
      "retryAfter": 16
    }
}

Context

Are you using the hosted instance of probot/settings or running your own?

Running a full sync using Github Action

Version of probot/settings

2.1.14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant