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

fix: Change from IgnoreAccessDeniedServiceDisabled to IgnoreCommonErrors #1033

Merged
merged 1 commit into from
Jun 13, 2022

Conversation

bbernays
Copy link
Contributor

@bbernays bbernays commented Jun 10, 2022

🎉 Thank you for making CloudQuery awesome by submitting a PR 🎉

Summary

When we use IgnoreAccessDeniedServiceDisabled we have to handle certain errors like 404/ Resource not found errors in the resolvers. By changing to IgnoreCommonErrors we get that at the table level without any extra code.

I am not 100% sure this is valid or if there are cases when we do NOT want to ignore 404s


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 19:33
@bbernays bbernays requested review from disq and yevgenypats and removed request for a team June 10, 2022 19:33
Copy link
Contributor

@roneli roneli left a comment

Choose a reason for hiding this comment

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

LGTM

Regarding the edge cases, in general the resolver still needs to handle not founds specially in cases where its listing and doing on each on an action.

Root resolver: resolves 100 resources

  • relation resolver: for each resource resolve - if not found (with new ignore error + using root table ignore error, it will just not resolve and not be inserted in DB), same with columns

On the other hand on Root Resolver if we do this logic and return on a not found we won't resolve the next 100 resources, so it should continue to resolve if it gets a not found.

@disq
Copy link
Member

disq commented Jun 13, 2022

I was clear for me but now I'm all confused 😂

@bbernays bbernays merged commit 1b98229 into cloudquery:main Jun 13, 2022
kodiakhq bot pushed a commit that referenced this pull request Jun 14, 2022
🤖 I have created a release *beep* *boop*
---


## [0.12.10](v0.12.9...v0.12.10) (2022-06-13)


### Bug Fixes

* Added `not found` errors handlers in athena resources ([#1021](#1021)) ([f18abef](f18abef))
* Added `not found` errors handlers in athena resources ([#1021](#1021)) ([297fa6c](297fa6c))
* change from IgnoreAccessDeniedServiceDisabled to IgnoreCommonErrors ([#1033](#1033)) ([1b98229](1b98229))
* Redshift Panic when accessing nested attributes ([8bae50b](8bae50b))
* Update endpoints ([#1032](#1032)) ([bd6159a](bd6159a))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
erezrokah pushed a commit to cloudquery/cloudquery that referenced this pull request Aug 14, 2022
🤖 I have created a release *beep* *boop*
---


## [0.12.10](cloudquery/cq-provider-aws@v0.12.9...v0.12.10) (2022-06-13)


### Bug Fixes

* Added `not found` errors handlers in athena resources ([#1021](cloudquery/cq-provider-aws#1021)) ([f18abef](cloudquery/cq-provider-aws@f18abef))
* Added `not found` errors handlers in athena resources ([#1021](cloudquery/cq-provider-aws#1021)) ([297fa6c](cloudquery/cq-provider-aws@297fa6c))
* change from IgnoreAccessDeniedServiceDisabled to IgnoreCommonErrors ([#1033](cloudquery/cq-provider-aws#1033)) ([1b98229](cloudquery/cq-provider-aws@1b98229))
* Redshift Panic when accessing nested attributes ([8bae50b](cloudquery/cq-provider-aws@8bae50b))
* Update endpoints ([#1032](cloudquery/cq-provider-aws#1032)) ([bd6159a](cloudquery/cq-provider-aws@bd6159a))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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