Safe Settings doesn't detect removed rules from repository rulesets

JakubBiel commented 2 months ago

Problem Description

When one or more rules are manually removed from a ruleset or when required status checks are modified, Safe Settings doesn't detect any changes. Interestingly, it does detect changes when rules are added, ruleset name is changed or bypass actors are modified.

What is actually happening

The app doesn't recognise changes when rules are removed from a repository ruleset.

What is the expected behavior

If someone manually removes the rules the app should revert those changes and add the rules back in.

Error output, if available

{"level":20,"time":1720003088766,"pid":25,"hostname":"safe-settings-app","name":"probot","name":"probot","name":"event","id":"54ec46e4-3928-11ef-944d-baa8cbbf33a3","msg":"Results of comparing Rulesets diffable target [{\"id\":1071805,\"name\":\"Custom-Bypass\",\"target\":\"branch\",\"source_type\":\"Repository\",\"source\":\"<redacted>/test-data-product\",\"enforcement\":\"active\",\"conditions\":{\"ref_name\":{\"exclude\":[],\"include\":[\"~DEFAULT_BRANCH\"]}},\"rules\":[{\"type\":\"deletion\"}],\"node_id\":\"RRS_lACqUmVwb3NpdG9yec4ut6w-zgAQWr0\",\"created_at\":\"2024-07-01T15:18:56.620Z\",\"updated_at\":\"2024-07-01T15:18:56.620Z\",\"bypass_actors\":[],\"current_user_can_bypass\":\"never\",\"_links\":{\"self\":{\"href\":\"https://api.github.com/repos/<redacted>/test-data-product/rulesets/1071805\"},\"html\":{\"href\":\"https://github.com/<redacted>/test-data-product/rules/1071805\"}}}] with source [{\"name\":\"Custom-Bypass\",\"target\":\"branch\",\"enforcement\":\"active\",\"bypass_actors\":[],\"conditions\":{\"ref_name\":{\"include\":[\"~DEFAULT_BRANCH\"],\"exclude\":[]}},\"rules\":[{\"type\":\"non_fast_forward\"},{\"type\":\"deletion\"},{\"type\":\"required_linear_history\"},{\"type\":\"pull_request\",\"parameters\":{\"dismiss_stale_reviews_on_push\":true,\"require_code_owner_review\":true,\"require_last_push_approval\":true,\"required_approving_review_count\":1,\"required_review_thread_resolution\":true}},{\"type\":\"required_status_checks\",\"parameters\":{\"strict_required_status_checks_policy\":true,\"required_status_checks\":[{\"context\":\"run-lint\"}]}}]}] is [object Object]"}
{"level":20,"time":1720003088766,"pid":25,"hostname":"safe-settings-app","name":"probot","name":"probot","name":"event","id":"54ec46e4-3928-11ef-944d-baa8cbbf33a3","msg":"There are no changes for Rulesets for repo [object Object]. Skipping changes"}

Context

Tried both on suborg and repo level

rulesets:
  - name: Somename
    target: branch
    enforcement: active
    conditions:
        ref_name:
          include: ["~DEFAULT_BRANCH"]
          exclude: []
    rules:
      - type: non_fast_forward
      - type: deletion
      - type: required_linear_history
      - type: pull_request
        parameters:
          dismiss_stale_reviews_on_push: true
          require_code_owner_review: true
          require_last_push_approval: true
          required_approving_review_count: 1
          required_review_thread_resolution: true
      - type: required_status_checks
        parameters:
          strict_required_status_checks_policy: true
          required_status_checks:
            - context: run-lint

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

Own deployment running v2.1.10

If running your own instance, are you using it with github.com or GitHub Enterprise?

github.com

Version of probot/settings

2.1.10

Version of GitHub Enterprise

n/a

JakubBiel commented 3 weeks ago

Thanks @luvsaxena1 It did fix the issue when rules are being removed but didn't fix the one with required status checks being changed.

Logs

{"level":20,"time":1724754180685,"pid":25,"hostname":"safe-settings-app","name":"probot","name":"probot","name":"event","id":"537f9070-645e-11ef-8f67-434e793c26ec","msg":"Results of comparing Rulesets diffable target [{\"id\":1515742,\"name\":\"Test ruleset\",\"target\":\"branch\",\"source_type\":\"Repository\",\"source\":\"test/test\",\"enforcement\":\"active\",\"conditions\":{\"ref_name\":{\"exclude\":[],\"include\":[\"~DEFAULT_BRANCH\"]}},\"rules\":[{\"type\":\"deletion\"},{\"type\":\"required_linear_history\"},{\"type\":\"pull_request\",\"parameters\":{\"required_approving_review_count\":1,\"dismiss_stale_reviews_on_push\":true,\"require_code_owner_review\":true,\"require_last_push_approval\":true,\"required_review_thread_resolution\":true}},{\"type\":\"merge_queue\",\"parameters\":{\"merge_method\":\"MERGE\",\"max_entries_to_build\":5,\"min_entries_to_merge\":1,\"max_entries_to_merge\":5,\"min_entries_to_merge_wait_minutes\":5,\"grouping_strategy\":\"ALLGREEN\",\"check_response_timeout_minutes\":60}},{\"type\":\"non_fast_forward\"},{\"type\":\"required_status_checks\",\"parameters\":{\"strict_required_status_checks_policy\":true,\"do_not_enforce_on_create\":false,\"required_status_checks\":[]}}],\"node_id\":\"RRS_lACqUmVwb3NpdG9yec4ut6w-zgAXIN4\",\"created_at\":\"2024-08-27T09:49:08.113Z\",\"updated_at\":\"2024-08-27T09:49:08.113Z\",\"bypass_actors\":[],\"current_user_can_bypass\":\"never\",\"_links\":{\"self\":{\"href\":\"https://api.github.com/repos/test/test/rulesets/1515742\"},\"html\":{\"href\":\"https://github.com/test/test/rules/1515742\"}}}] with source [{\"name\":\"Test ruleset\",\"target\":\"branch\",\"enforcement\":\"active\",\"conditions\":{\"ref_name\":{\"include\":[\"~DEFAULT_BRANCH\"],\"exclude\":[]}},\"rules\":[{\"type\":\"non_fast_forward\"},{\"type\":\"deletion\"},{\"type\":\"required_linear_history\"},{\"type\":\"pull_request\",\"parameters\":{\"dismiss_stale_reviews_on_push\":true,\"require_code_owner_review\":true,\"require_last_push_approval\":true,\"required_approving_review_count\":1,\"required_review_thread_resolution\":true}},{\"type\":\"merge_queue\",\"parameters\":{\"check_response_timeout_minutes\":60,\"grouping_strategy\":\"ALLGREEN\",\"max_entries_to_build\":5,\"max_entries_to_merge\":5,\"merge_method\":\"MERGE\",\"min_entries_to_merge\":1,\"min_entries_to_merge_wait_minutes\":5}},{\"type\":\"required_status_checks\",\"parameters\":{\"strict_required_status_checks_policy\":true,\"required_status_checks\":[{\"context\":\"run-lint\"}]}}]}] is [object Object]"}

JakubBiel commented 3 weeks ago

Also, there's another issue when there are required_status_checks specified on two levels (e.g. suborg and repo) for the same ruleset. Instead of merging the list of required status checks it adds another rule to the ruleset so there's an error when creating a ruleset - Rulesets | HttpError: Validation Failed: "Invalid rules: 'Required status checks . Strangely, this kind of works when updating the ruleset and the last provided required_status_checks is applied (repo level).

github / safe-settings