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

Safe Settings doesn't detect removed rules from repository rulesets #655

Open
JakubBiel opened this issue Jul 3, 2024 · 2 comments
Open
Labels
bug Something isn't working

Comments

@JakubBiel
Copy link

JakubBiel commented Jul 3, 2024

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
Copy link
Author

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
Copy link
Author

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: &quot;Invalid rules: &#39;Required status checks . Strangely, this kind of works when updating the ruleset and the last provided required_status_checks is applied (repo level).

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