-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
add aws_appfabric_ingestion resource #37291
base: main
Are you sure you want to change the base?
Conversation
Community NoteVoting for Prioritization
For Submitters
|
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.
Welcome @davelemons 👋
It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTOR guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.
Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.
Thanks again, and welcome to the community! 😃
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.
Good code, excellent start. Couple of minor things
- Update Function
- Tests -- hard coding needs to be removed and more tests need to be added.
- change log file is required.
}, | ||
}, | ||
"arn": schema.StringAttribute{ | ||
Optional: true, |
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.
can you consider a framework attribute?
Optional: true, | ||
Computed: true, | ||
}, | ||
"app_bundle_identifier": schema.StringAttribute{ |
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.
Alphabetical order should be considered, for ex...app_bundle before arn..
return | ||
} | ||
|
||
in := &appfabric.CreateIngestionInput{ |
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 can consider using framework expand function to auto expand which is the preferred way.
return | ||
} | ||
|
||
plan.App = flex.StringToFramework(ctx, out.Ingestion.App) |
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.
Consider using Framework function to simplify this.
resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...) | ||
} | ||
|
||
func (r *resourceIngestion) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) { |
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.
Read comes before update. Please re-arrange the sequence if possible.
resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...) | ||
} | ||
|
||
func (r *resourceIngestion) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) { |
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 need to revisit this a bit, consider looking at other update services like bedrock / bedrockagent examples and refactor if possible plz.
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.
Good callout...the AppFabric resources don't have specific update calls so, I was following the advice from Skaff about using the same logic in create. Bedrock Agent has a specific update call. What is the recommendation for services without an update, or do you have a similar service I could look at?
return | ||
} | ||
|
||
state.App = flex.StringToFramework(ctx, out.App) |
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 can consider using the framework flatten to auto this.
{ | ||
ResourceName: resourceName, | ||
ImportState: true, | ||
ImportStateIdFunc: testAccIngestionImportStateIDFunc(ctx, resourceName), |
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.
Guess we can remove this test
resource.ParallelTest(t, resource.TestCase{ | ||
PreCheck: func() { | ||
acctest.PreCheck(ctx, t) | ||
testAccPreCheck(ctx, 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.
Kindly follow existing standards.
Thank you for your contribution! 🚀 A new usage of AWS SDK for Go V1 was detected. Please prefer AWS SDK for Go V2 for all net-new services. If this is an enhancement or bug fix to an existing AWS SDK Go V1 based resource, this comment can be safely ignored. For additional information refer to the AWS SDK for Go Versions page in the contributor guide. |
I believe I have all of the items addressed. There is no update API, so I changed it to a no-op, let me know if there is anything else I should do there. It's a pretty simple service and I'm struggling to think of other tests to add at this point and open to some suggestions/pointers of additional tests to add. Also, I'm not sure why it's seeing GO V1 usage...I should be using V2 for everything. |
Description
This PR adds a resource for aws_appfabric_ingestion
Relations
Relates #34549
References
AppFabric Ingestion Docs
Output from Acceptance Testing