Skip to content

fix(gcp): Fix context to work when syncing#5512

Merged
kodiakhq[bot] merged 6 commits intomainfrom
fix/gcp_context
Dec 8, 2022
Merged

fix(gcp): Fix context to work when syncing#5512
kodiakhq[bot] merged 6 commits intomainfrom
fix/gcp_context

Conversation

@yevgenypats
Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats commented Dec 8, 2022

Seems ctx never worked given that ctx is passed on creation and not on call.

Also, this takes some learning from azure and makes code generation easier.

For context there is no need for services anymore and we can create and close the server in the testing.go helper function instead of doing this in every single mock function

Also, make mocks and codegen easier with learnings from Azure
@cq-bot cq-bot added the gcp label Dec 8, 2022
@yevgenypats yevgenypats mentioned this pull request Dec 8, 2022
@yevgenypats yevgenypats marked this pull request as ready for review December 8, 2022 15:54
@yevgenypats yevgenypats requested a review from disq as a code owner December 8, 2022 15:54
Copy link
Copy Markdown
Contributor

@hermanschaaf hermanschaaf left a comment

Choose a reason for hiding this comment

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

The fix makes sense to me, and I spot-checked a few resources - looks good, but I didn't go through it line by line :)

ClientOptions []option.ClientOption
// All gcp services initialized by client
Services *Services
// Services *Services
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: let's completely remove this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the Services struct still used anywhere after this? Can it be removed? (I don't have the code checked out locally right now to check)

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

Nice

@yevgenypats yevgenypats added the automerge Automatically merge once required checks pass label Dec 8, 2022
@kodiakhq kodiakhq bot merged commit 81c8075 into main Dec 8, 2022
@kodiakhq kodiakhq bot deleted the fix/gcp_context branch December 8, 2022 18:14
kodiakhq bot pushed a commit that referenced this pull request Dec 13, 2022
🤖 I have created a release *beep* *boop*
---


## [4.0.0](plugins-source-gcp-v3.2.0...plugins-source-gcp-v4.0.0) (2022-12-13)


### ⚠ BREAKING CHANGES

* **gcp:** Only list enabled Services ([#5557](#5557))
* **gcp:** Only List Project Roles ([#5556](#5556))
* **gcp:** `private_key_type` column was removed from the `gcp_iam_service_account_keys` table as it was always populated with `nil`. If you were using it in one of your queries, you can safely remove it.

### Features

* **gcp:** Add retrier ([#5522](#5522)) ([bf8c212](bf8c212)), closes [#5514](#5514)


### Bug Fixes

* **deps:** Update module github.com/cloudquery/plugin-sdk to v1.11.1 ([#5458](#5458)) ([58b7432](58b7432))
* **deps:** Update module github.com/cloudquery/plugin-sdk to v1.11.2 ([#5497](#5497)) ([c1876cf](c1876cf))
* **deps:** Update module github.com/cloudquery/plugin-sdk to v1.12.0 ([#5539](#5539)) ([fb71293](fb71293))
* **gcp:** Fix context to work when syncing ([#5512](#5512)) ([81c8075](81c8075))
* **gcp:** Only list enabled Services ([#5557](#5557)) ([5310bef](5310bef))
* **gcp:** Only List Project Roles ([#5556](#5556)) ([51acb48](51acb48))
* **gcp:** Remove `private_key_type` column from `gcp_iam_service_account_keys` ([1371928](1371928))

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

automerge Automatically merge once required checks pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants