-
Notifications
You must be signed in to change notification settings - Fork 43
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
Provider properties interface #4151
Conversation
@JAORMX there are no tests, but manual testing works fine. I think the code is ready for review, but I won't take it out of draft until we have tests |
RepoPropertyName = "name" | ||
RepoPropertyOwner = "owner" | ||
RepoPropertyRepoID = "repo_id" | ||
// TODO: is this GH-specific? If yes, it should be moved under a provider-specific property |
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.
Let's add the GitHub specific constants under GitHub. This should only have "standard" ones. e.g. we can keep "is_private", "is_archived", "is_fork".
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.
done, there's a separate patch now with just constants for generic entity that tracks with your PR #4178 then a bunch of provider-agnostic repo properties and then the rest of the properties have been moved under the provider
RepoPropertyIsArchived = "is_archived" | ||
RepoPropertyIsFork = "is_fork" | ||
RepoPropertyDefaultBranch = "default_branch" | ||
RepoPropertyLanguages = "properties.languages" |
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.
I'm unsure about properties.languages
. Yes, we need languages, I just wanna figure out what's a good way to express this property.
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.
I removed them for now. I think we need to figure out a better way to express the nested properties anyway, this was too much of a hack
b2bfa56
to
f71c408
Compare
Adds a simple API to wrap any value in a Property type and a map of values as Properties with type-safe getters.
Adds a new provider interface to retrieve a single property or all properties. Related: mindersec#4165
We'll want to store certain attributes for all entities as well as certain attribtues for all repositories. This patch adds those as constants. Related: mindersec#4165
Implements the `FetchAllProperties` and `FetchProperty` methods of the `PropertiesFetcher` interface for the github provider. Related: mindersec#4165
Instead of fething a github-specific repo object, just fetch the properties using the new `PropertiesFetcher` interface and use them to construct the repository protobuf and store in the DB. Related: mindersec#4165
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
Summary
This PR adds a new provider interface for fetching properties as well as its implementation for the github provider. The interface returns new Property types which are convenience types to allow returning Go types from the unstructured properties returned by the providers. Lastly, the PR adds constants for the currently used properties, either generic entity properties, generic repo properties or github-specific repo properties.
Closes: #4165
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: