feat: add prefix filter to team sync groups data source - #3248
feat: add prefix filter to team sync groups data source#3248laughedelic wants to merge 4 commits into
Conversation
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
deiga
left a comment
There was a problem hiding this comment.
Thanks for the PR. I left some comments that need addressing
deiga
left a comment
There was a problem hiding this comment.
Sorry, the ID will need to change and thus we need a schema migration
stevehipwell
left a comment
There was a problem hiding this comment.
@laughadelic please could you rebase this PR, please note that we've changed how the docs work and ideally want to generate the docs for this data source via the automation.
e21fe5d to
c2041b9
Compare
stevehipwell
left a comment
There was a problem hiding this comment.
Thanks for updating @laughedelic, I've added some review comments.
| } | ||
|
|
||
| if v, ok := d.GetOk("prefix_filter"); ok { | ||
| options.Query = v.(string) |
There was a problem hiding this comment.
| options.Query = v.(string) | |
| q, _ := v.(string) | |
| options.Query = q |
Otherwise you'll get a lint failure.
| "github.com/hashicorp/terraform-plugin-testing/tfjsonpath" | ||
| ) | ||
|
|
||
| func TestAccGithubOrganizationTeamSyncGroupsDataSource_existing(t *testing.T) { |
There was a problem hiding this comment.
| func TestAccGithubOrganizationTeamSyncGroupsDataSource_existing(t *testing.T) { | |
| func TestAccGithubOrganizationTeamSyncGroupsDataSource(t *testing.T) { |
Please nest sub-tests using t.Run instead of using steps for separate tests. I suggest the following sub tests.
all- Single step
filtered- Step 1 a filter that matches
- Step 2 change the filter to match nothing
|
|
||
| ## Example Usage | ||
|
|
||
| Retrieve all IdP groups: |
There was a problem hiding this comment.
Please can you update the docs to use the template pattern of automatically inserting the examples and using the schema for the attribute descriptions, meaning this description should be a comment in the example file.
| } | ||
|
|
||
| d.SetId(fmt.Sprintf("%s/github-org-team-sync-groups", orgName)) | ||
| id, err := buildID(orgName, options.Query) |
There was a problem hiding this comment.
This might be clearer if we use a placeholder when query isn't set.
There was a problem hiding this comment.
Done in de55491. The ID is now <org>:* when no filter is set, and <org>:<prefix> otherwise
| "prefix_filter": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Description: "Filters the results to return only those that begin with the specified value.", |
There was a problem hiding this comment.
I'm assuming there are rules on the naming of team sync groups? If so we need to validate here.
There was a problem hiding this comment.
I don't think there's anything to validate here. These names come from the external identity provider rather than from GitHub, so the permitted characters are whatever the IdP allows. Entra ID group names, Okta group names, etc. From my experience with Okta, there isn't any expected format.
Adds an optional prefix_filter argument to github_organization_team_sync_groups that filters returned IdP groups to those whose names begin with the given value. Uses buildID(orgName, options.Query) for the resource ID. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Avoid a lint failure by using a checked type assertion when reading the prefix_filter value. - Use "*" as an explicit placeholder in the data source ID when no prefix filter is set, so the ID always has the same shape. - Restructure the acceptance test into `all` and `filtered` sub-tests using t.Run instead of packing unrelated cases into steps. The `filtered` sub-test first applies a matching prefix, then switches to a prefix that matches nothing. - Switch the docs template to the standard tfplugindocs pattern: the examples are inserted automatically from the example files and the attribute documentation comes from the schema. The prose describing each example moved into the example files as comments.
The previous hand-written template was replaced with the standard tfplugindocs pattern in the previous commit, but the examples were not picked up because they lived in a directory named after the short data source name. tfplugindocs discovers them under examples/data-sources/<full_data_source_name>/data-source*.tf, so the examples move to examples/data-sources/github_organization_team_sync_groups/ and are renamed accordingly. With the description moved onto the schema, the per-data-source template no longer adds anything over templates/data-sources.md.tmpl, so it is removed and the page is now fully generated.
c2041b9 to
d2915b1
Compare
Before the change?
After the change?
Now it's possible to filter groups by prefix:
Pull request checklist
Does this introduce a breaking change?
I followed the API and the SDK in naming itq, but I'm open to something more descriptive, e.g.prefixorprefix_filter