Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

⚡️ verify github connection only once #4980

Merged
merged 3 commits into from
Dec 16, 2024
Merged

Conversation

afiune
Copy link
Contributor

@afiune afiune commented Dec 14, 2024

Supersedes #4976

When a user scans a single GH repository, this was not a problem, but for users that scan an entire organization with thousands of repositories, we were creating a connection for each asset and validating it thousands of times.

This changes the behavior to verify the connection only once which will save us thousands of GH API requests.

When a user scans a single GH repository, this was not a problem, but
for users that scan an entire organization with thousands of
repositories, we were creating a connection for each asset and
validating it thousands of times.

This changes the behavior to verify the connection only once which will
save us thousands of GH API requests.

Signed-off-by: Salim Afiune Maya <[email protected]>
Copy link
Contributor

github-actions bot commented Dec 14, 2024

Test Results

3 171 tests  ±0   3 169 ✅ ±0   1m 50s ⏱️ -4s
  379 suites ±0       2 💤 ±0 
   29 files   ±0       0 ❌ ±0 

Results for commit 533e7f1. ± Comparison against base commit fee2739.

♻️ This comment has been updated with latest results.

@@ -38,25 +40,75 @@ type GithubConnection struct {
asset *inventory.Asset
client *github.Client
ctx context.Context

// Used to avoid verifying a client with the same options more than once
Hash uint64
Copy link
Member

Choose a reason for hiding this comment

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

nit: can we call this optionsHash or something similar to make its purpose more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll rename it in a diff PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afiune afiune merged commit f3eafd0 into main Dec 16, 2024
17 checks passed
@afiune afiune deleted the afiune/validate-gh-conn-once branch December 16, 2024 13:47
@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2024
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.

2 participants