Skip to content

feat(aws): Use PreResourceResolver to resolve list/describe resources#2461

Merged
kodiakhq[bot] merged 5 commits intocloudquery:mainfrom
disq:feat/aws_use_preresource_resolver
Oct 7, 2022
Merged

feat(aws): Use PreResourceResolver to resolve list/describe resources#2461
kodiakhq[bot] merged 5 commits intocloudquery:mainfrom
disq:feat/aws_use_preresource_resolver

Conversation

@disq
Copy link
Copy Markdown
Member

@disq disq commented Oct 5, 2022 •
edited
Loading

Shouldn't change performance as we don't have the proposed "PreResourceResolver parallelism" in the plugin-sdk yet.

@cq-bot cq-bot added the aws label Oct 5, 2022
@disq disq requested review from amanenk and bbernays October 5, 2022 17:35
@disq disq mentioned this pull request Oct 5, 2022
Merged
@disq disq force-pushed the feat/aws_use_preresource_resolver branch 3 times, most recently from 878494e to ade8001 Compare October 6, 2022 14:22
@hermanschaaf
Copy link
Copy Markdown
Contributor

hermanschaaf commented Oct 6, 2022

@disq Looks like docs need to be regenerated here (the AWS workflow failed)

@disq disq force-pushed the feat/aws_use_preresource_resolver branch from ade8001 to 49a3ecd Compare October 6, 2022 15:17
@disq
Copy link
Copy Markdown
Member Author

disq commented Oct 6, 2022

@disq Looks like docs need to be regenerated here (the AWS workflow failed)

nah it was because of the codegen drift in main, before merging #2476. fixed.

@disq disq force-pushed the feat/aws_use_preresource_resolver branch from 49a3ecd to a649bb6 Compare October 7, 2022 07:26
bbernays
bbernays reviewed Oct 7, 2022
View reviewed changes
plugins/source/aws/codegen/recipes/backup.go
SubService: "plans",
Struct: &backup.GetBackupPlanOutput{},
SkipFields: []string{"BackupPlanArn"},
PreResourceResolver: "getPlan",
Copy link
Copy Markdown
Collaborator

@bbernays bbernays Oct 7, 2022

Choose a reason for hiding this comment

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

Shouldn't the PreResourceResolver be a plural, because it is getting a list of items?

Copy link
Copy Markdown
Member Author

@disq disq Oct 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Pre is actually not a good name, because it's the last call before we "realize" a resource with its fields, after the actual resolver is called (to potentially get the list of items). The "singularization" of the list of items is done by the SDK, and we get a single handle from the SDK, look it up in the PreResourceResolver handler and do a resource.Item assignment (or resource.SetItem(), which is never used for some reason)

So it's actually a "ResourceAssignHook". We get the previously assigned data from resource.Item, do a bunch of things with it and look up the real/detailed data and set it to the same member.

@disq disq added the automerge Automatically merge once required checks pass label Oct 7, 2022
@kodiakhq kodiakhq bot merged commit f31ece8 into cloudquery:main Oct 7, 2022
@cq-bot cq-bot mentioned this pull request Oct 7, 2022
Merged
kodiakhq bot pushed a commit that referenced this pull request Oct 9, 2022
@cq-bot
chore(main): Release plugins-source-aws v2.1.0 (#2491)
3e138d6 
🤖 I have created a release *beep* *boop*
---


## [2.1.0](plugins-source-aws-v2.0.1...plugins-source-aws-v2.1.0) (2022-10-09)


### Features

* **aws:** Use PreResourceResolver to resolve list/describe resources ([#2461](#2461)) ([f31ece8](f31ece8))


### Bug Fixes

* **deps:** Update plugin-sdk for aws to v0.12.10 ([#2544](#2544)) ([4e4fdb6](4e4fdb6))
* **deps:** Update plugin-sdk for aws to v0.12.8 ([#2495](#2495)) ([ddb163e](ddb163e))
* **deps:** Update plugin-sdk for aws to v0.12.9 ([#2509](#2509)) ([cda0307](cda0307))
* Update endpoints ([#2490](#2490)) ([624e7a8](624e7a8))
* Update endpoints ([#2529](#2529)) ([f170e50](f170e50))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
@disq disq deleted the feat/aws_use_preresource_resolver branch October 12, 2022 09:27
kodiakhq bot pushed a commit that referenced this pull request Oct 12, 2022
@disq
feat(aws): Use PreResourceResolver when necessary, remove ListAndDeta…
340f614 
…ilResolver (#2460)

More here #2461

**BEWARE** merging this before updating the plugin-sdk for "PreResourceResolver parallelism" will cause performance regressions. (#2461 doesn't have that issue, as those resources are already linear)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbernays bbernays bbernays left review comments

@hermanschaaf hermanschaaf Awaiting requested review from hermanschaaf

@amanenk amanenk Awaiting requested review from amanenk

+1 more reviewer

@yevgenypats yevgenypats yevgenypats approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

automerge Automatically merge once required checks pass

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@disq @hermanschaaf @yevgenypats @bbernays @cq-bot