feat: Adding github_enterprise_ip_allow_list_entry resource#2649
feat: Adding github_enterprise_ip_allow_list_entry resource#2649ErikElkins wants to merge 31 commits intointegrations:mainfrom
Conversation
ErikElkins
changed the title
|
@ErikElkins Apologies for the delay on getting to this PR. Would you be willing to fix the lint issue? Thank you! |
stevehipwell
left a comment
stevehipwell
left a comment
There was a problem hiding this comment.
Is there a REST API for this functionality?
github-project-automation
bot
moved this from 👀 In review
to 🏗 In progress
in 🧰 Octokit Active
|
Yep! Let me jump in here. |
Co-authored-by: Steve Hipwell <steve.hipwell@gmail.com>
Co-authored-by: Steve Hipwell <steve.hipwell@gmail.com>
|
Oh, i see the other PR. Should we just close this? EDIT: NEVERMIND |
Doesn't look like they've added it since I opened the PR. |
gateixeira
left a comment
gateixeira
left a comment
There was a problem hiding this comment.
hi @ErikElkins 👋
I was updating your PR to have the lint fixed and realized that the acceptance tests seem to also not pass. Can you please update the PR with the following? Feel free to validate on your end.
Co-authored-by: gateixeira <4645845+gateixeira@users.noreply.github.com>
Co-authored-by: gateixeira <4645845+gateixeira@users.noreply.github.com>
Co-authored-by: gateixeira <4645845+gateixeira@users.noreply.github.com>
gateixeira
left a comment
gateixeira
left a comment
There was a problem hiding this comment.
@ErikElkins thanks for the changes!
Done! |
Co-authored-by: Steve Hipwell <steve.hipwell@gmail.com>
Co-authored-by: Steve Hipwell <steve.hipwell@gmail.com>
|
@ErikElkins could you please rebase? |
|
@deiga are you happy with this now? If so could you please approve (or clear your requested changes)? |
| var query struct { | ||
| Node struct { | ||
| IpAllowListEntry struct { | ||
| ID githubv4.String | ||
| AllowListValue githubv4.String | ||
| Name githubv4.String | ||
| IsActive githubv4.Boolean | ||
| CreatedAt githubv4.String | ||
| UpdatedAt githubv4.String | ||
| Owner struct { | ||
| Enterprise struct { | ||
| Slug githubv4.String | ||
| } `graphql:"... on Enterprise"` | ||
| } | ||
| } `graphql:"... on IpAllowListEntry"` | ||
| } `graphql:"node(id: $id)"` | ||
| } | ||
|
|
||
| variables := map[string]any{ | ||
| "id": githubv4.ID(d.Id()), | ||
| } | ||
|
|
||
| err := client.Query(ctx, &query, variables) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| entry := query.Node.IpAllowListEntry | ||
|
|
||
| if err := d.Set("enterprise_slug", string(entry.Owner.Enterprise.Slug)); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := d.Set("ip", string(entry.AllowListValue)); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := d.Set("name", entry.Name); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := d.Set("is_active", entry.IsActive); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := d.Set("created_at", entry.CreatedAt); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := d.Set("updated_at", entry.UpdatedAt); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
issue: Am I seeing things or did you re-add the API query to the Import func?
The ID should be enough as the Read func is the next thing to be called with futher Terraform operations and it does the exact same thing as Import. It doesn't seem useful to have an API interaction here
There was a problem hiding this comment.
I asked for this, the query is required otherwise it'll be recreated du to the IP being unset and the import is a complete waste of time.
There was a problem hiding this comment.
It was the recommended approach by @stevehipwell here: #2649 (comment)
There was a problem hiding this comment.
@stevehipwell Oh, right.
What if we add the IP to the import ID to circumvent the need for a API call? It's already QUITE difficult to run Import as finding the GQL ID of the allowlist entry requires one to query the API manually first 😬
Actually, I wonder if there is a way to make Import more user friendly? 🤔
There was a problem hiding this comment.
I was hoping to do the import based on ip, but IP is not unique in the IP allow list. You could have the same IP and description multiple times in the list for one enterprise. :(
EDIT: It's unique based on IP AND description
|
|
||
| err := client.Mutate(ctx, &mutation, input, nil) | ||
| if err != nil { | ||
| return diag.FromErr(err) |
There was a problem hiding this comment.
Please add error handling that a 404 doesn't unnecessarily create a Terraform error
There was a problem hiding this comment.
Added error handling and a comment. The GraphQL API (and github go client) means it'll look kinda janky. Let me know if you'd rather have me remove it.
5 participants
Resolves #2648
Before the change?
After the change?
github_enterprise_ip_allow_list_entryresource.Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!