Skip to content
This repository has been archived by the owner on Aug 16, 2022. It is now read-only.

feat: Add support for resource groups #1396

Merged
merged 9 commits into from
Aug 10, 2022

Conversation

adam-tylr
Copy link
Contributor

@adam-tylr adam-tylr commented Aug 7, 2022

Summary

Adds support for resource groups.


Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines πŸ§‘β€πŸŽ“
  • Run go fmt to format your code πŸ–Š
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests. Learn more about testing here πŸ§ͺ
  • Update the docs by running go run ./docs/docs.go and committing the changes πŸ“ƒ
  • Ensure the status checks below are successful βœ…

@adam-tylr adam-tylr requested a review from a team as a code owner August 7, 2022 14:51
@adam-tylr adam-tylr requested review from bbernays and removed request for a team August 7, 2022 14:51
@erezrokah erezrokah changed the title Add support for resource groups feat: Add support for resource groups Aug 7, 2022
@adam-tylr
Copy link
Contributor Author

I have one question, resource groups have a single field, description which is not returned by the "list" API. I would need to call the "get" API for each group to get that field. Should I add that?

@bbernays
Copy link
Contributor

bbernays commented Aug 7, 2022

It is common in AWS APIs that the List call doesn't return very much information. Here is an example of the best way to handle the List + Describe pattern

func fetchKinesisStreams(ctx context.Context, meta schema.ClientMeta, parent *schema.Resource, res chan<- interface{}) error {

@bbernays
Copy link
Contributor

bbernays commented Aug 9, 2022

This looks good. Only a couple of small issues

  1. I think you need to rerun the docs generation
  2. Some linting of the go code...

disq
disq previously requested changes Aug 9, 2022
Copy link
Member

@disq disq left a comment

Choose a reason for hiding this comment

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

Few nits around error handling

@adam-tylr
Copy link
Contributor Author

This looks good. Only a couple of small issues

  1. I think you need to rerun the docs generation
  2. Some linting of the go code...

Reran the docs, for the linting error, that struct is placed at the bottom by cq-gen. I moved it manually but it will move back if this file is regenerated

@bbernays
Copy link
Contributor

bbernays commented Aug 9, 2022

Reran the docs, for the linting error, that struct is placed at the bottom by cq-gen. I moved it manually but it will move back if this file is regenerated

@erezrokah @hermanschaaf @roneli Any insight or suggestion on how to proceed?

@hermanschaaf
Copy link
Member

@bbernays @adam-tylr We decided to disable the decorder check on resource files for now: 34fa4d6 (It's related to this issue: cloudquery/.github#266)

@adam-tylr If you bring your branch up-to-date with main, re-generate the code and then push it back up, I expect the build should pass. Sorry for the trouble πŸ™‡

@adam-tylr
Copy link
Contributor Author

Brought up to date with main and re-generated. What is the check-new-files-have-check-for-changes-flag.sh doing? Seems to still be failing

@bbernays bbernays requested a review from disq August 10, 2022 16:31
@bbernays bbernays dismissed disq’s stale review August 10, 2022 16:31

Issues have been addressed

@bbernays bbernays merged commit dc6aeab into cloudquery:main Aug 10, 2022
@adam-tylr adam-tylr deleted the feature/resourcegroups branch August 10, 2022 23:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants