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

fix: Ignore invalid subscriptions #421

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zagronitay
Copy link
Contributor

Summary

closes cloudquery/cloudquery-issues#434

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 βœ…

@zagronitay zagronitay requested a review from a team as a code owner July 19, 2022 11:14
@zagronitay zagronitay requested review from amanenk and removed request for a team July 19, 2022 11:14
Copy link
Contributor

@amanenk amanenk left a comment

Choose a reason for hiding this comment

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

LGTM

for i, subscriptionId := range client.subscriptions {
if !validateSubscriptionId(subscriptionId) {
logger.Error("Invalid subscription id", "subscription_id", subscriptionId)
client.subscriptions = append(client.subscriptions[:i], client.subscriptions[i+1:]...)
Copy link
Member

Choose a reason for hiding this comment

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

I believe you can run into trouble if you remove elements from a slice while also ranging over it. Here's a playground demo to illustrate. What you can do, though, is this trick from the SliceTricks page: https://github.com/golang/go/wiki/SliceTricks#filtering-without-allocating (that's my good example in the playground)

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.

None yet

3 participants