-
Notifications
You must be signed in to change notification settings - Fork 123
Add security list resource #1489
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
Conversation
560756c to
ac6107a
Compare
ac6107a to
e15e70d
Compare
|
@copilot Implement the above suggestions |
|
@nick-benoit I've opened a new pull request, #1490, to work on those changes. Once the pull request is ready, I'll request review from you. |
a4efc07 to
1bfcb71
Compare
1bfcb71 to
358c970
Compare
| - Prefer using existing util functions over longer form, duplicated code: | ||
| - `utils.IsKnown(val)` instead of `!val.IsNull() && !val.IsUnknown()` | ||
| - `utils.ListTypeAs` instead of `val.ElementsAs` or similar for other collection types | ||
| - The final state for a resource should be derived from a read request following a mutative request (eg create or update). We should not use the response from a mutative request to build the final resource state. |
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.
Copilot kept suggesting that I use the create / update response instead of doing a secondary read. Adding this seems to help with that.
358c970 to
708a26c
Compare
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.
Pull request overview
Copilot reviewed 22 out of 24 changed files in this pull request and generated 3 comments.
|
|
||
| switch resp.StatusCode() { | ||
| case http.StatusOK: | ||
| return resp, nil |
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.
| return resp, nil | |
| return resp.JSON200, nil |
Does it make sense to return the parsed response models from these helpers?
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 it does. I may opt to do that refactor in the list item PR though for the sake of easily pulling these changes into that dependent PR.
| if apiList.UnderscoreVersion != nil { | ||
| m.VersionID = types.StringValue(string(*apiList.UnderscoreVersion)) | ||
| } else { | ||
| m.VersionID = types.StringNull() | ||
| } |
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.
We have this pattern a lot where the API returns a custom string type and so we can't use types.StringPointerValue.
Want to create a new typesutil package with something like:
func StringishPointerValue[T ~string](ptr *T) types.String {
if ptr == nil {
return types.StringNull()
}
return types.StringValue(string(*ptr))
}so this code can just be
| if apiList.UnderscoreVersion != nil { | |
| m.VersionID = types.StringValue(string(*apiList.UnderscoreVersion)) | |
| } else { | |
| m.VersionID = types.StringNull() | |
| } | |
| m.VersionID = typesutil.StringishPointerValue(apiList.UnderscoreVersion) |
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 seemed like these utilities fit pretty well into the existing utils/utils.go file. Happy to add a dedicated package if you prefer that though.
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.
Happy to leave this for a follow up PR. A catch-all utils package is discouraged by the Golang gods, so I was thinking we could start moving these TF type specific helpers to a dedicated typeutils package.
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'll make a note and add that to the security list item PR 👍
ec0c547 to
d89ae58
Compare
| listID := state.ListID.ValueString() | ||
|
|
||
| // Try to parse as composite ID from state.ID | ||
| if compId, diags := clients.CompositeIdFromStrFw(state.ID.ValueString()); !diags.HasError() { |
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.
tobio
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.
One last change
| if apiList.Deserializer != nil { | ||
| m.Deserializer = utils.StringishPointerValue(apiList.Deserializer) | ||
| } else { | ||
| m.Deserializer = types.StringNull() | ||
| } |
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.
We don't need the if/else for these with this new helper
| if apiList.Deserializer != nil { | |
| m.Deserializer = utils.StringishPointerValue(apiList.Deserializer) | |
| } else { | |
| m.Deserializer = types.StringNull() | |
| } | |
| m.Deserializer = utils.StringishPointerValue(apiList.Deserializer) |
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.
Ahh good call: 1907787
tobio
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.
🎉
Adds Security List resource Docs.
This is supporting work for Security Exceptions.
On it's own this resource is not tremendously useful, but will quickly be followed by PRs for the following: