Skip to content

feat: Added store fetch summary routine#356

Merged
roneli merged 23 commits intocloudquery:mainfrom
amanenk:fetch_summary
Jan 17, 2022
Merged

feat: Added store fetch summary routine#356
roneli merged 23 commits intocloudquery:mainfrom
amanenk:fetch_summary

Conversation

@amanenk
Copy link
Copy Markdown
Contributor

@amanenk amanenk commented Dec 21, 2021

closes #284

@amanenk amanenk requested review from disq, irmatov and roneli December 21, 2021 12:29
Copy link
Copy Markdown
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.

Didn't post this review for some reason, sorry for the delay. Looks like a good start, have some structural changes to purpose + we need to support migrate for core tables that are created, otherwise it will be hard to manage it.

extended fetch summary structure
changelog
@amanenk amanenk requested review from disq and roneli December 28, 2021 07:30
Co-authored-by: Kemal <223029+disq@users.noreply.github.com>
Copy link
Copy Markdown
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.

Looking good, had a few extra comments

# Conflicts:
#	pkg/client/client.go
# Conflicts:
#	pkg/client/migrations/1_v0.17.4.down.sql
@amanenk amanenk requested review from disq and roneli December 30, 2021 15:01
Copy link
Copy Markdown
Contributor

@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.

I would run the migration on fetch and not on new Client because otherwise we will have to have a database running even for commands that do not require a database such as cloudquery policy describe and potentially others.

@roneli
Copy link
Copy Markdown
Contributor

roneli commented Jan 2, 2022

I would run the migration on fetch and not on new Client because otherwise we will have to have a database running even for commands that do not require a database such as cloudquery policy describe and potentially others.

I would add there are some places that client can be initialized without dsn otherwise it tries to connect, I think also in policy describe it tries. So if dsn != "" its safe to run migrations

Copy link
Copy Markdown
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.

More comments I missed in prior review

Copy link
Copy Markdown
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.

Another comment I missed

amanenk and others added 4 commits January 10, 2022 09:30
Co-authored-by: Ron <38083777+roneli@users.noreply.github.com>
Co-authored-by: Ron <38083777+roneli@users.noreply.github.com>
# Conflicts:
#	go.mod
#	go.sum
@amanenk
Copy link
Copy Markdown
Contributor Author

amanenk commented Jan 10, 2022

I would run the migration on fetch and not on new Client because otherwise we will have to have a database running even for commands that do not require a database such as cloudquery policy describe and potentially others.

I would add there are some places that client can be initialized without dsn otherwise it tries to connect, I think also in policy describe it tries. So if dsn != "" its safe to run migrations

If we will have some other core tables related to other commands it is better to migrate in the client initialization

@amanenk amanenk requested review from roneli and yevgenypats January 11, 2022 11:28
Copy link
Copy Markdown
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, two minor nits

amanenk and others added 2 commits January 17, 2022 10:12
Co-authored-by: Ron <38083777+roneli@users.noreply.github.com>
Co-authored-by: Ron <38083777+roneli@users.noreply.github.com>
@amanenk amanenk changed the title added store fetch summary routine feat: Added store fetch summary routine Jan 17, 2022
@amanenk amanenk requested a review from roneli January 17, 2022 09:57
@roneli roneli merged commit d6e24db into cloudquery:main Jan 17, 2022
TinLe pushed a commit to TinLe/cloudquery that referenced this pull request Jan 19, 2022
* upstream/main:
  chore: Synced local '.github/workflows/' with remote 'workflows/common' (cloudquery#421)
  fix: Don't show telemetry notice when it's not enabled (cloudquery#418)
  feat: Increase ulimit in unix environment (cloudquery#416)
  feat: Support build-schema for all providers in config (cloudquery#414)
  fix: Fetch summary fixes (cloudquery#417)
  feat: Added store fetch summary routine (cloudquery#356)
  chore: Synced file(s) with cloudquery/.github (cloudquery#415)
  feat: Expose max_parallel_resource_fetch_limit (cloudquery#413)
  feat: Adjust log messages when .cq dir is absent (cloudquery#411)
  fix: Adjust policy describe message (cloudquery#409)
  fix: Exit with 1 on policy error (cloudquery#412)
erezrokah pushed a commit that referenced this pull request Aug 14, 2022
🤖 I have created a release *beep* *boop*
---


## [0.11.11](cloudquery/cq-provider-azure@v0.11.10...v0.11.11) (2022-06-22)


### Features

* YAML config support ([#353](cloudquery/cq-provider-azure#353)) ([045e92f](cloudquery/cq-provider-azure@045e92f))

---
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 that referenced this pull request Aug 14, 2022
🤖 I have created a release *beep* *boop*
---


## [0.8.15](cloudquery/cq-provider-gcp@v0.8.14...v0.8.15) (2022-06-22)


### Features

* YAML config support ([#352](cloudquery/cq-provider-gcp#352)) ([23ddfb4](cloudquery/cq-provider-gcp@23ddfb4))

---
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 join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Store Fetch Summary

4 participants