Allow to filter out archived repositories in the github enterprise connector #109
Conversation
…nnector Add a configuration option to do this for the GitHub connector by filtering out repositories returned (using the 'archived' attribute) - there is no API-level filter for this. This works for both the GitHub Enterprise and GitHub v2 connectors.
WalkthroughAdds a new boolean configuration option to omit archived GitHub repositories, wires it from generated config through connector initialization into repository builder, and uses it to skip archived repositories during listing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Config as Config (gen + runtime)
participant Connector as Connector.New
participant RepoBuilder as repositoryBuilder
participant GitHubAPI as GitHub API
Note over Config,Connector: configuration creation & wiring
Config->>Connector: ghc.OmitArchivedRepositories
Connector->>RepoBuilder: repositoryBuilder(..., omitArchivedRepositories)
RepoBuilder->>GitHubAPI: List repositories (paginated)
alt omitArchivedRepositories == true
GitHubAPI->>RepoBuilder: repos...
RepoBuilder->>RepoBuilder: filter out archived repos
else
GitHubAPI->>RepoBuilder: repos...
end
RepoBuilder->>Connector: resources (filtered)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/connector/repository_test.go (1)
27-27: Consider adding test coverage for the filtering behavior.The test correctly updates the call to match the new signature. However, there's no test that verifies archived repositories are actually filtered out when
omitArchivedRepositoriesis set totrue.Consider adding a test case similar to this:
t.Run("should filter archived repositories when flag is true", func(t *testing.T) { mgh := mocks.NewMockGitHub() githubOrganization, archivedRepo, activeRepo := mgh.SeedWithArchivedRepo() githubClient := github.NewClient(mgh.Server()) cache := newOrgNameCache(githubClient) // Test with flag enabled clientWithFilter := repositoryBuilder(githubClient, cache, true) organization, _ := organizationResource(ctx, githubOrganization, nil, false) resources, _, _, err := clientWithFilter.List(ctx, organization.Id, &pagination.Token{}) require.Nil(t, err) // Verify archived repo is not in the list for _, res := range resources { require.NotEqual(t, archivedRepo.GetID(), res.Id.Resource) } // Test with flag disabled clientWithoutFilter := repositoryBuilder(githubClient, cache, false) resources, _, _, err = clientWithoutFilter.List(ctx, organization.Id, &pagination.Token{}) require.Nil(t, err) // Verify archived repo IS in the list found := false for _, res := range resources { if res.Id.Resource == fmt.Sprint(archivedRepo.GetID()) { found = true break } } require.True(t, found, "archived repository should be included when filter is disabled") })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/config/conf.gen.go(1 hunks)pkg/config/config.go(2 hunks)pkg/connector/connector.go(3 hunks)pkg/connector/repository.go(3 hunks)pkg/connector/repository_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/config/config.go (1)
vendor/github.com/conductorone/baton-sdk/pb/c1/config/v1/config.pb.go (3)
BoolField(566-572)BoolField(585-585)BoolField(600-602)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🔇 Additional comments (7)
pkg/connector/repository.go (3)
63-63: LGTM!The field is correctly added to the struct and will be used to control filtering behavior.
109-111: LGTM!The filtering logic correctly skips archived repositories when the flag is enabled. The implementation uses
repo.GetArchived()to check the archived status and continues to the next repository when both the flag is true and the repository is archived.
416-422: LGTM!The function signature is correctly updated to accept the
omitArchivedRepositoriesparameter and properly initializes the field in the returned struct.pkg/config/conf.gen.go (1)
12-12: LGTM!The generated configuration field is correctly defined with the appropriate mapstructure tag that matches the configuration field name.
pkg/connector/connector.go (3)
101-101: LGTM!The field is correctly added to the GitHub struct to store the configuration option.
110-110: LGTM!The flag is properly passed to the
repositoryBuilder, ensuring the filtering behavior is applied during repository synchronization.
319-319: LGTM!The field is correctly initialized from the configuration, completing the wiring from config to runtime behavior.
pkg/config/config.go
Outdated
| ) | ||
| omitArchivedRepositories = field.BoolField( | ||
| "omit-archived-repositories", | ||
| field.WithDisplayName("Omit syncying archived repositories"), |
There was a problem hiding this comment.
| field.WithDisplayName("Omit syncying archived repositories"), | |
| field.WithDisplayName("Omit syncing archived repositories"), |
Now we know that it exists :) Also fix some grammar/spelling
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/connector/repository.go (1)
59-64: Consider adding a documentation comment.The new
omitArchivedRepositoriesfield would benefit from a brief comment explaining that it controls client-side filtering of archived repositories (since the GitHub API doesn't provide this filter).Apply this diff to add documentation:
type repositoryResourceType struct { resourceType *v2.ResourceType client *github.Client orgCache *orgNameCache + // omitArchivedRepositories controls whether to filter out archived repositories + // during listing. Filtering is done client-side as the GitHub API does not + // provide a native parameter for this. omitArchivedRepositories bool }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/config/conf.gen.go(1 hunks)pkg/config/config.go(2 hunks)pkg/connector/connector.go(2 hunks)pkg/connector/repository.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connector/connector.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/config/config.go (1)
vendor/github.com/conductorone/baton-sdk/pb/c1/config/v1/config.pb.go (3)
BoolField(566-572)BoolField(585-585)BoolField(600-602)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🔇 Additional comments (4)
pkg/config/config.go (2)
46-50: LGTM! Typo has been corrected.The field definition is correct and the typo from the previous review ("syncying" → "syncing") has been properly addressed.
71-71: LGTM!The field is properly added to the configuration field set.
pkg/config/conf.gen.go (1)
1-90: LGTM! Generated code is correct.The generated
OmitArchivedRepositoriesfield properly maps to the configuration field defined inconfig.go, and the mapstructure tag is correctly set.pkg/connector/repository.go (1)
416-422: LGTM!The
repositoryBuilderfunction signature is correctly updated to accept the newomitArchivedRepositoriesparameter, and the struct initialization properly wires it through. Since this is an unexported function, the breaking change is limited to package-internal callers (which have been updated according to the AI summary).
| if o.omitArchivedRepositories && repo.GetArchived() { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! Verify test coverage for the filtering behavior.
The filtering logic correctly skips archived repositories when the flag is enabled. The implementation properly handles the case where archived repos are fetched from the API but filtered client-side (as noted in the PR description, there's no API-level filter available).
Run the following script to verify that tests exist for the archived repository filtering behavior:
🏁 Script executed:
#!/bin/bash
# Description: Check for test coverage of the omitArchivedRepositories filtering logic
# Search for test functions that exercise the filtering behavior
rg -n "omitArchivedRepositories" --type=go -g '*_test.go' -C 5
# Look for tests that verify archived repositories are filtered
rg -n -i "archived.*repositor" --type=go -g '*_test.go' -C 3Length of output: 0
Let me search more broadly for test coverage:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find test files related to repository.go
fd -e '_test.go' --path '*repository*'
# Look for all test files in connector package
fd -e '_test.go' . | grep -i connector
# Search for List method tests (broader pattern)
rg -n "func.*Test.*List" --type=go -g '*_test.go'
# Check if repository.go has any tests at all
rg -n "func.*Test.*Repositor" --type=go -g '*_test.go'
Length of output: 370
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the repository_test.go file
cat pkg/connector/repository_test.go
# Also check if there are specific tests for the List method with filtering
rg -n "archived" --type=go pkg/connector/repository_test.go
Length of output: 2056
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the List method in repository.go to understand what needs testing
rg -n "func.*List" --type=go -B 2 -A 20 pkg/connector/repository.go | head -60
Length of output: 671
---
🏁 Script executed:
```shell
#!/bin/bash
# Get more of the List method, focusing on the filtering logic around lines 109-111
rg -n "func.*List" --type=go -B 2 -A 50 pkg/connector/repository.go | head -90
Length of output: 1472
Test coverage required for the omitArchivedRepositories filtering logic.
The filtering implementation is correct, but there are currently no tests for the List method or the omitArchivedRepositories flag. Tests must be added to cover:
- List method correctly returns repositories when omitArchivedRepositories is false
- List method filters out archived repositories when omitArchivedRepositories is true
- Pagination still works correctly when repositories are filtered out
🤖 Prompt for AI Agents
In pkg/connector/repository.go around lines 109 to 111, add unit tests for the
List method to cover omitArchivedRepositories behavior: create test cases that
(1) set omitArchivedRepositories=false and verify archived and non-archived
repos are returned, (2) set omitArchivedRepositories=true and verify archived
repos are excluded, and (3) verify pagination still returns correct pages when
archived repos are filtered out (e.g., create >pageSize repos with mixed
archived flags, request specific pages and assert contents and counts). Use the
existing test harness/mocks for the repository store, seed deterministic repo
objects with GetArchived() true/false, assert total counts and per-page results,
and include edge cases (no repos, all archived, all unarchived).
Add a configuration option to do this for the GitHub connector by filtering out repositories returned (using the 'archived' attribute) - there is no API-level filter for this. This works for both the GitHub Enterprise and GitHub v2 connectors.
https://conductorone.atlassian.net/browse/DUCT-13273
https://conductorone.atlassian.net/browse/BB-1583
Summary by CodeRabbit