Skip to content

fix(policy): Use firebase instead of GitHub to get latest version#618

Merged
erezrokah merged 4 commits into
cloudquery:mainfrom
erezrokah:fix/github_rate_limit
May 2, 2022
Merged

fix(policy): Use firebase instead of GitHub to get latest version#618
erezrokah merged 4 commits into
cloudquery:mainfrom
erezrokah:fix/github_rate_limit

Conversation

@erezrokah
Copy link
Copy Markdown
Member

Fixes https://github.com/cloudquery/cloudquery-issues/issues/324 (internal issue).

This PR gets the policy latest version from Firebase instead of GitHub to avoid hitting rate limits.
To reproduce run cloudquery policy run github::github.com/cloudquery-policies/aws multiple times.

This is the smallest change I could think of to fix the issue.

Example data https://firestore.googleapis.com/v1/projects/hub-cloudquery/databases/(default)/documents/orgs/cloudquery/policies/aws/versions?mask.fieldPaths=policy.Name

@github-actions github-actions Bot added the fix label May 2, 2022
}

valid := make([]*version.Version, 0)
for _, d := range doc.Documents {
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.

This is how our UI does it https://github.com/cloudquery/cq-website/blob/ef3e714533610edfbd50d842d87bd13019dcad91/hub/src/services/firebase.ts#L37 (internal link).

We could make the data easier to consume, but that will probably be a breaking change for our Website

Comment thread internal/getter/github.go

var (
repoToFirebasePath = map[string]string{
"cloudquery-policies": "cloudquery",
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.

Mapping between the repo name to the Firebase path

}

func (h Hub) getLatestRelease(ctx context.Context, organization, providerName string) (string, error) {
versions, err := url.Parse(fmt.Sprintf(h.url+"/versions", organization, providerName))
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.

Extracted to a new Firebase struct

@erezrokah
Copy link
Copy Markdown
Member Author

Looking at the failed tests

ExpectedFound bool
ExpectedError error
}{
{
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.

The test_policy is not in our Hub

@erezrokah erezrokah marked this pull request as ready for review May 2, 2022 06:55
@erezrokah erezrokah requested review from a team and disq and removed request for a team May 2, 2022 06:55
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.

One nit

Comment thread internal/firebase/client.go Outdated
url string
}

func NewFirebaseClient(registryUrl string) *FirebaseClient {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
func NewFirebaseClient(registryUrl string) *FirebaseClient {
func New(registryUrl string) *Client {

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.

Thanks for the review! Good point, done in 1c8f776

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Missed the type rename though 😂

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.

Ah, good catch. Done in #622

@erezrokah erezrokah merged commit 455ed23 into cloudquery:main May 2, 2022
@erezrokah erezrokah deleted the fix/github_rate_limit branch May 2, 2022 08:13
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.

2 participants