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

fix: Continue fetching on incorrect account permissions #1030

Merged
merged 9 commits into from
Jun 10, 2022

Conversation

bbernays
Copy link
Contributor

πŸŽ‰ Thank you for making CloudQuery awesome by submitting a PR πŸŽ‰

Summary


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 πŸ“ƒ
  • If adding a new resource, add relevant Terraform files in a separate PR πŸ“‚
  • Ensure the status checks below are successful βœ…

@bbernays bbernays requested a review from a team as a code owner June 10, 2022 01:39
@bbernays bbernays requested review from roneli and removed request for a team June 10, 2022 01:39
Copy link
Member

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

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

Looks good. This code section in general might need additional refactor in the future but for the sake of unblocking this and fixing number of issues that block customers we can push this now.

Just some idea - remove InitServicesForPartitionAccountAndScope as we only need to create aws clients per region per account rather per some parameter - Otherwise we are not alligned with AWS server side throttling mechanism and we will get throttle more.

@yevgenypats yevgenypats changed the title Improve Error Handling in AWS Client fix: Continue fetching on incorrect account permissions Jun 10, 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.

Why the partition business? A credential only be valid in a single partition, no? So why not have the partition as a property in Client and get rid of the additional map levels?

@bbernays
Copy link
Contributor Author

Why the partition business? A credential only be valid in a single partition, no? So why not have the partition as a property in Client and get rid of the additional map levels?

@disq - We need to store the partition in that serviceregion map because the client.partition field can be overwritten if there are credentials from different partitions

@bbernays
Copy link
Contributor Author

Looks good. This code section in general might need additional refactor in the future but for the sake of unblocking this and fixing number of issues that block customers we can push this now.

Just some idea - remove InitServicesForPartitionAccountAndScope as we only need to create aws clients per region per account rather per some parameter - Otherwise we are not alligned with AWS server side throttling mechanism and we will get throttle more.

@yevgenypats - Completely agree that more code can be refactored, but was trying to balance changing code with fixing issues. I would not delete InitServicesForPartitionAccountAndScope in this pr as I would want to take a deeper look into that change

@bbernays bbernays linked an issue Jun 10, 2022 that may be closed by this pull request
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.

Looks like Account struct can be removed now, LGTM otherwise

@bbernays bbernays merged commit 71008d2 into cloudquery:main Jun 10, 2022
}
return accounts
Copy link
Contributor

Choose a reason for hiding this comment

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

do we care about duplicates here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think an AWS account can span multiple partitions

@bbernays bbernays deleted the improve-aws-config-initialization branch December 7, 2022 17:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants