-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[INS-206] Store Gitlab Project ID in secret location metadata #4601
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
base: main
Are you sure you want to change the base?
[INS-206] Store Gitlab Project ID in secret location metadata #4601
Conversation
amanfcp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! A few suggestions
| // Cache project metadata if available. | ||
| if gitUnit, ok := unit.(git.SourceUnit); ok && gitUnit.Metadata != nil { | ||
| if gitUnit.Metadata["project_id"] != "" { | ||
| project := &project{} | ||
| projectId, err := strconv.Atoi(gitUnit.Metadata["project_id"]) | ||
| if err != nil { | ||
| ctx.Logger().Error(err, "could not convert project_id metadata to int", "project_id", gitUnit.Metadata["project_id"]) | ||
| } else { | ||
| project.id = projectId | ||
| } | ||
| project.name = gitUnit.Metadata["project_name"] | ||
| project.owner = gitUnit.Metadata["project_owner"] | ||
| s.repoToProjCache.set(repoURL, project) | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would recommend early return flow here. It's kind of difficult to read 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, totally agree. I'll make the changes
| if proj.Owner != nil { | ||
| metadata["project_owner"] = proj.Owner.Email | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're missing the username check here as handled in gitlabProjectToCacheProject
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch, thanks
…data' of mustansir:mustansir14/trufflehog into INS-206-Store-GitLab-project-ID-in-secret-location-metadata
Description:
This PR adds the changes to include gitlab project details like project ID, project name and project owner name to the metadata of a chunk.
Achieving this wasn't straightforward as the chunks are generated by the
gitsource and it calls an injectedSourceMetadataFuncto create the metadata. The signature of this function obviously does not include the gitlab project ID.The solution implemented here is to maintain a
repoToProjectCachemap that stores the project details for a repo. This cache is populated as we enumerate the repos and the callback function uses this cache to populate the project details.The only concerning part of this solution is memory usage, so just to put in perspective how much this impacts that, I found a public organization with ~3000 projects and ran benchmarks for before and after making the changes. (I know this isn't anywhere close to the largest organizations we've come across, but this is the biggest one I could find that was public)
Results before making changes:
Total memory usage: 43234992 bytes (43.23 MBs)
Results after this change:
Total memory usage: 44666216 bytes (44.66 MBs)
It's important to note that this doesn't really perform a full scan (I tried doing that first but it would take hours), so I tweaked the code to expose the callback function and directly called that after enumerating the repos. This is the code used for benchmarking:
Checklist:
make test-community)?make lintthis requires golangci-lint)?