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

Add missing tags #191

Merged
merged 34 commits into from
Oct 19, 2021
Merged

Add missing tags #191

merged 34 commits into from
Oct 19, 2021

Conversation

amanenk
Copy link
Contributor

@amanenk amanenk commented Sep 10, 2021

fixes #157

@amanenk amanenk marked this pull request as ready for review September 21, 2021 13:32
roneli
roneli suggested changes Sep 23, 2021
View reviewed changes
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.

Partial Review since this is a BIG PR.

  • left go over migrations again
  • distribution resources (check if you dropped tables etc')

resources/apigatewayv2_vpc_links.go Outdated Show resolved Hide resolved
resources/ecs_clusters.go Show resolved Hide resolved
resources/elbv2_load_balancers.go Show resolved Hide resolved
resources/elbv2_load_balancers.go Outdated Show resolved Hide resolved
resources/elasticsearch_domains.go Show resolved Hide resolved
resources/elasticbeanstalk_environments.go Show resolved Hide resolved
resources/elasticbeanstalk_environments.go Outdated Show resolved Hide resolved
resources/migrations/5_v0.5.14.down.sql Outdated Show resolved Hide resolved
resources/migrations/5_v0.5.14.up.sql Outdated Show resolved Hide resolved
resources/migrations/5_v0.5.14.up.sql Outdated Show resolved Hide resolved
roneli
roneli suggested changes Oct 4, 2021
View reviewed changes
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.

Can we move the cloudfront distributions into a separate PR ? since its a breaking change.

@amanenk amanenk requested a review from roneli October 11, 2021 10:16
roneli
roneli reviewed Oct 14, 2021
View reviewed changes
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, would add IgnoreError checks for tag resolvers with API calls for access denied

CHANGELOG.md Outdated Show resolved Hide resolved
@amanenk amanenk requested a review from roneli October 14, 2021 15:33
@amanenk
Copy link
Contributor Author

amanenk commented Oct 15, 2021

There is no specific errors for tag requests so all the tags errors will be shown as warnings

roneli
roneli suggested changes Oct 19, 2021
View reviewed changes
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.

Minor change requests and we can merge

resources/migrations/5_v0.5.14.down.sql Outdated Show resolved Hide resolved
@amanenk amanenk requested a review from roneli October 19, 2021 12:20
roneli
roneli approved these changes Oct 19, 2021
View reviewed changes
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

@roneli roneli merged commit 02e40f0 into cloudquery:main Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@roneli roneli roneli approved these changes

@zagronitay zagronitay Awaiting requested review from zagronitay

@irmatov irmatov Awaiting requested review from irmatov

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

Successfully merging this pull request may close these issues.

Tags missing
2 participants
@amanenk @roneli