-
Notifications
You must be signed in to change notification settings - Fork 7
660 add rate limit configuration resource #1176
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
base: main
Are you sure you want to change the base?
660 add rate limit configuration resource #1176
Conversation
henryrecker-pingidentity
left a comment
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.
Looks good 👍
patrickcping
left a comment
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.
Couple of minor things but LGTM 🚀 🚀
| grantTypes, | ||
| management.EnumApplicationOIDCTokenAuthMethod(plan.TokenEndpointAuthnMethod.ValueString()), | ||
| ) | ||
| data.SetGrantTypes(grantTypes) |
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's worth migrating this change to #1184
| ) | ||
|
|
||
| createdAtDescription := framework.SchemaAttributeDescriptionFromMarkdown( | ||
| "A string that specifies the time the resource was created.", |
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 might be good to mention the formatting of the date (e.g. "An RFC 3339 formatted string...") for this and updated_at
| } | ||
|
|
||
| // Save updated data into Terraform state | ||
| resp.Diagnostics.Append(data.toState(response)...) |
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.
I think we need to include a if resp.Diagnostics.HasError() ... check between this toState and the following resp.State.Set for the "CRU" operations. I remember seeing an edge case once that led to panic. This check may be on some resources but probably not applied universally yet
| }, | ||
| "DeleteRateLimitConfiguration", | ||
| legacysdk.CustomErrorResourceNotFoundWarning, | ||
| sdk.DefaultCreateReadRetryable, |
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.
Probably don't need the ..CreateReadRetryable here, nil should work fine on deletes
| }, | ||
| }) | ||
| } | ||
|
|
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.
I think we need a func TestAccRateLimitConfiguration_NewEnv(t *testing.T) { ... } test to catch new environment eventual consistency race conditions (see other environment "root level" resources as an example). We do something like it in the "RemovalDrift" test, but that's not the intention of that test so errors can be misinterpreted
Change Description
CDI-660: Add
pingone_rate_limit_configurationresourceChange Characteristics
Checklist
All full (or complete) PRs that need review prior to merge should have the following box checked.
If contributing a partial or incomplete change (expecting the development team to complete the remaining work) please leave the box unchecked
Required SDK Upgrades
github.com/patrickcping/pingone-go-sdk-v2/management v0.63.0Testing
This PR has been tested with:
Shell Command(s)
Testing Results
Expand Results
End-to-end Tests Workflow Links