Skip to content

chore: Add suppress diff func for unordered list#3514

Open
stevehipwell wants to merge 2 commits into
integrations:mainfrom
stevehipwell:add-suppress-list-func
Open

chore: Add suppress diff func for unordered list#3514
stevehipwell wants to merge 2 commits into
integrations:mainfrom
stevehipwell:add-suppress-list-func

Conversation

@stevehipwell

Copy link
Copy Markdown
Collaborator

Resolves #ISSUE_NUMBER


Before the change?

  • Suppressing unordered list diffs required implementing for each type

After the change?

  • Added suppressUnorderedListDiff function that supports suppressing diffs for lists using identity based ordering
  • github_organization_ruleset & github_repository_ruleset use the new function

Pull request checklist

  • Schema migrations have been created if needed (example)
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

@stevehipwell stevehipwell self-assigned this Jul 2, 2026
@stevehipwell stevehipwell added the Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR label Jul 2, 2026
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

👋 Hi, and thank you for this contribution!

This repo is maintained by GitHub and community members on a best-effort basis. We'll get to this as soon as we can.

You can help us prioritize by joining the discussion on open issues and PRs, sharing details on the changes you need, and reviewing other contributions.


🤖 This is an automated message.

deiga
deiga previously approved these changes Jul 2, 2026
Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

These provider review instructions are being used.

Findings

  1. MEDIUM — bypass_actors diff suppression may not work for ID-less actors (plan diffs can persist)

    • File reference: github/util_rules.go:908
    • actor_id is optional in the schema (and may be omitted / non-int in the decoded map), but bypassActorCompareIdentity currently returns 0 whenever actor_id isn’t an int, which prevents deterministic ordering for those entries and can defeat the unordered-diff suppression.
    • Suggested fix: use the existing toInt helper and include bypass_mode as a tiebreaker (inline fix provided in comment).
  2. MEDIUM — New unit test doesn’t assert the negative cases

    • File reference: github/util_rules_test.go:1450
    • Test_bypassActorCompareIdentity only fails when wantSame=true; all wantSame=false cases currently can’t fail, reducing its value as a regression test.
    • Suggested fix: add an else assertion for wantSame=false (inline fix provided in comment).

Changes:

  • Added a reusable suppressUnorderedListDiff helper for suppressing diffs on unordered TypeList fields via a caller-provided comparator.
  • Replaced ruleset bypass_actors diff suppression to use the new helper and a dedicated bypass-actor comparator.
  • Added a unit test for the bypass-actor comparator.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
github/util_diff.go Adds generic unordered-list diff suppression helper used by schema fields.
github/util_rules.go Replaces bypass-actors diff suppression logic with a comparator-based identity sort.
github/util_rules_test.go Adds unit coverage for the bypass-actor comparator.
github/resource_github_repository_ruleset.go Switches bypass_actors to use suppressUnorderedListDiff.
github/resource_github_organization_ruleset.go Switches bypass_actors to use suppressUnorderedListDiff.

Comment thread github/util_rules.go
Comment thread github/util_rules_test.go

@deiga deiga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

@stevehipwell stevehipwell added this to the v6.14.0 milestone Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants