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

Property service for saving properties to database #4248

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Aug 22, 2024

Summary

Adds a service that either returns cached properties for a very short amount of time or contacts the provider to fetch updated service.

The service also marshalls the properties to the database properly.

Fixes: #4171

Change Type

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

mostly unit tests so far

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

if err != nil {
return 0, fmt.Errorf("value is not of type int64: %w", err)
}
return int64(fval), nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This need more work because internally structpb.Value stores any numerical value as float64. In order to handle int64 and uint64 I will, when creating a property or properties, check if the leaf value is either of those and change the value to be an object containing our own type annotation and the stringified value. The reverse on getting the value.

@coveralls
Copy link

coveralls commented Aug 22, 2024

Coverage Status

coverage: 53.913% (+0.1%) from 53.764%
when pulling 05dc819 on jhrozek:prop_svc
into d32c80c on stacklok:main.

)

// PropertiesService is the interface for the properties service
type PropertiesService interface {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I wanted to have just the Retrieve methods that would save the entity as well when saving the properties. But - I don't know if we can generalize saving the entity, especially while we are still in migrating mode and are saving the entity with a defined UUID that we take from the older tables. So at least for now I split the methods into separate retrieve and save where the idea is that the caller would retrieve properties, save the entity and then save the properties.

internal/db/entity_helpers.go Outdated Show resolved Hide resolved
internal/entities/properties/properties.go Outdated Show resolved Hide resolved
internal/entities/properties/service/service.go Outdated Show resolved Hide resolved
@jhrozek jhrozek force-pushed the prop_svc branch 2 times, most recently from 03e26bc to 0696eb4 Compare August 23, 2024 15:11
@jhrozek
Copy link
Contributor Author

jhrozek commented Aug 23, 2024

@JAORMX this version should be getting quite close. I need to check the unit tests and such, but the manual tests work and the interface is I think what I'd like it to have. I'll be polishing this and then un-draft the PR. I'll also send the first patches in the PR separately for review so we can upstream them and avoid conflicts.

@jhrozek
Copy link
Contributor Author

jhrozek commented Aug 23, 2024

I fixed a couple of tests, but this is as far as I go on a Friday afternoon. Pieces of the PR have been submitted separately and apart from the tests, I think this is ready to go.

Adds a service that either returns cached properties for a very short
amount of time or contacts the provider to fetch updated service.

The service also marshalls the properties to the database properly.

Fixes: mindersec#4171
@jhrozek jhrozek changed the title WIP: Property service for saving properties to database Property service for saving properties to database Aug 26, 2024
@jhrozek jhrozek marked this pull request as ready for review August 26, 2024 08:24
@JAORMX JAORMX merged commit a277db7 into mindersec:main Aug 26, 2024
23 checks passed
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.

Add properties fetching for artifacts in the GitHub provider
3 participants