Skip to content

fix: Don't multiplex subscriptions#2018

Merged
kodiakhq[bot] merged 4 commits intocloudquery:mainfrom
erezrokah:fix/azure_subscriptions
Oct 3, 2022
Merged

fix: Don't multiplex subscriptions#2018
kodiakhq[bot] merged 4 commits intocloudquery:mainfrom
erezrokah:fix/azure_subscriptions

Conversation

@erezrokah
Copy link
Copy Markdown
Member

@erezrokah erezrokah commented Sep 22, 2022

Summary

Fixes #1947

We shouldn't be multiplexing subscriptions as the API returns all subscriptions. So we were basically getting all subscriptions for each subscription, and then using the client subscription id to set the value in the DB. Since we use ID as the primary key and do an upsert (at least in v2), the last operation is the one that "took".

I think this also means subscription locations should not be multiplexed either and be a relation of subscriptions.
Most of the changes are to support relations in the new SDK list pager API.

Removed the relations stuff to simplify things

Still a draft as needs some cleanup, which I'll do next week

Done


Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines 🧑‍🎓
  • Test locally on your own infrastructure
  • Run go fmt to format your code 🖊
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests 🧪
  • Ensure the status checks below are successful ✅

@cq-bot cq-bot added the azure label Sep 22, 2022
@erezrokah
Copy link
Copy Markdown
Member Author

I think this also means subscription locations should not be multiplexed either and be a relation of subscriptions. Most of the changes are to support relations in the new SDK list pager API.

Not sure about that, maybe since we have the subscriptions already we can make it a top level table

@erezrokah erezrokah force-pushed the fix/azure_subscriptions branch from 5bb0f00 to 69f55f9 Compare September 28, 2022 14:47
@erezrokah erezrokah force-pushed the fix/azure_subscriptions branch from 69f55f9 to d167add Compare September 28, 2022 14:48
Multiplex: client.SingleSubscriptionMultiplex,
Columns: []schema.Column{
{
Name: "subscription_id",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Subscriptions already have the id in ID so no need for subscription_id

@erezrokah erezrokah marked this pull request as ready for review September 28, 2022 14:49
@erezrokah erezrokah changed the title fix: Don't multiplex subscriptions, make locations releation fix: Don't multiplex subscriptions Sep 28, 2022
@erezrokah erezrokah requested review from hermanschaaf and removed request for yevgenypats September 28, 2022 14:51
@erezrokah
Copy link
Copy Markdown
Member Author

@hermanschaaf marked this as ready and made you reviewer :) I mad the minimal changes to fix the bug and I think we can do the relations bit if we feel that's the right approach, or required by users.

@erezrokah erezrokah added the automerge Automatically merge once required checks pass label Sep 29, 2022
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.

@erezrokah Looks good 👍 I couldn't test it locally the other day due to some unrelated local environment issues, but this fixes the bug so let's not block on that--we can always revisit if something still doesn't work right.

@kodiakhq kodiakhq bot merged commit 94d43b3 into cloudquery:main Oct 3, 2022
@erezrokah erezrokah deleted the fix/azure_subscriptions branch October 3, 2022 10:53
kodiakhq bot pushed a commit that referenced this pull request Oct 3, 2022
🤖 I have created a release *beep* *boop*
---


## [1.0.2-pre.0](plugins-source-azure-v1.0.1-pre.0...plugins-source-azure-v1.0.2-pre.0) (2022-10-03)


### Bug Fixes

* **deps:** Update module github.com/cloudquery/plugin-sdk to v0.11.2 ([#2162](#2162)) ([5701aa5](5701aa5))
* **deps:** Update module github.com/cloudquery/plugin-sdk to v0.11.4 ([#2210](#2210)) ([760d0a6](760d0a6))
* **deps:** Update module github.com/cloudquery/plugin-sdk to v0.11.5 ([#2227](#2227)) ([7db2dde](7db2dde))
* Don't multiplex subscriptions ([#2018](#2018)) ([94d43b3](94d43b3))

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

Investigation request(Azure): Wrong subscriptions id

3 participants