Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(secure-onboarding) Adding AccountComponent Resource #504

Merged
merged 6 commits into from
Apr 26, 2024
Merged

feat(secure-onboarding) Adding AccountComponent Resource #504

merged 6 commits into from
Apr 26, 2024

Conversation

ravinadhruve10
Copy link
Contributor

@ravinadhruve10 ravinadhruve10 commented Apr 24, 2024 •
edited
Loading

Change summary:

  • Adding new Account Component resource with schema and CRUD operations in parity with Cloudauth Account resource.
  • Adding the respective client and support for new resource type.
  • Refactored common parsing methods for conversion from/to component metadata.
  • Added TF ACC tests for the new resource type.
  • Added docs md for the new resource

@ravinadhruve10
feat(secure-onboarding) Adding AccountComponent Resource
72746c0 
Change summary:
----------------
- Adding new Account Component resource with schema
  and CRUD operations in parity with Cloudauth Account
  resource.
- Adding the respective client and support for new resource type.
- Refactored common parsing methods for conversion from/to
  component metadata.
- Added TF ACC tests for the new resource type.
@ravinadhruve10
Fix lint
7c9f29f
cgeers
cgeers approved these changes Apr 25, 2024
View reviewed changes
Copy link
Contributor

@cgeers cgeers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this largely looks good. I understand there are some functional cases that need to be addressed, but largely, this approach seems sound. Leaving a ✅ here even though it's likely more changes will come (and a rebase is needed).

@ravinadhruve10 ravinadhruve10 marked this pull request as ready for review April 26, 2024 02:51
cgeers
cgeers reviewed Apr 26, 2024
View reviewed changes
sysdig/resource_sysdig_secure_cloud_auth_account.go
declare common schemas used across resources here
*/
var (
accountComponents = &schema.Resource{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should this be singular?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually yes, singular makes sense for SchemaComponent usage in both account and component resources. I'll fix this when I touch this part of code again.

cgeers
cgeers approved these changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@cgeers cgeers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great @ravinadhruve10

@cgeers cgeers merged commit b33d7dd into master Apr 26, 2024
23 checks passed
@cgeers cgeers deleted the feat/onboarding/comp-resource branch April 26, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgeers cgeers cgeers approved these changes

@haresh-suresh haresh-suresh Awaiting requested review from haresh-suresh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ravinadhruve10 @cgeers