-
Notifications
You must be signed in to change notification settings - Fork 188
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
chore: Upgrades access_list_api_key resource to auto-generated SDK #1877
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
internal/service/accesslistapikey/data_source_accesslist_api_keys.go
Outdated
Show resolved
Hide resolved
"github.com/mongodb/terraform-provider-mongodbatlas/internal/testutil/mig" | ||
) | ||
|
||
func TestAccMigrationProjectAccesslistAPIKey_SettingIPAddress(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one doubt for the plural data source
accessListAPIKeys, _, err := conn.AccessListAPIKeys.List(ctx, orgID, apiKeyID, options) | ||
params := &admin.ListApiKeyAccessListsEntriesApiParams{ | ||
PageNum: pointy.Int(d.Get("page_num").(int)), | ||
ItemsPerPage: pointy.Int(d.Get("items_per_page").(int)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this now defines a pointer to the int value of the attribute, would there be a risk we send items per page to 0 if this attribute is not defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the attribute is not sent, the API should apply the default value (0
) anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're totally right!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify the risk is in ItemsPerPage which has a default value of 100, if a pointer value to 0 is defined the response will return 0 items when the attribute is not defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah thanks 👍 , I thought we were talking about page_num
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was working with items_per_page = 0, i suppose the server was assuming the default value. anyway it's fixed now to send nil (so not sent to server) if values are 0
|
||
_, resp, err := conn.AccessListAPIKeys.Create(ctx, orgID, apiKeyID, createRequest) | ||
_, resp, err := connV2.ProgrammaticAPIKeysApi.CreateApiKeyAccessList(context.Background(), orgID, apiKeyID, accessList).Execute() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fortunately this is not CFN :)
_, resp, err := connV2.ProgrammaticAPIKeysApi.CreateApiKeyAccessList(context.Background(), orgID, apiKeyID, accessList).Execute() | |
_, resp, err := connV2.ProgrammaticAPIKeysApi.CreateApiKeyAccessList(ctx, orgID, apiKeyID, accessList).Execute() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx, i missed that one 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!!
|
||
accessListAPIKeys, _, err := conn.AccessListAPIKeys.List(ctx, orgID, apiKeyID, options) | ||
params := &admin.ListApiKeyAccessListsEntriesApiParams{ | ||
PageNum: conversion.IntPtr(d.Get("page_num").(int)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AgustinBettati this sends nil if int is 0, as we want here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Upgrades access_list_api_key resource to auto-generated SDK.
Migration tests created.
Link to any related issue(s): https://jira.mongodb.org/browse/CLOUDP-226074
Type of change:
Required Checklist:
Further comments