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

Add migration for data sources. #5008

Merged
merged 1 commit into from
Nov 21, 2024
Merged

Add migration for data sources. #5008

merged 1 commit into from
Nov 21, 2024

Conversation

blkt
Copy link
Contributor

@blkt blkt commented Nov 20, 2024

Summary

This migration adds support for three additional tables, data_sources and data_sources_functions containing the definition of a single data source in a many-to-one relationship, and a rule_type_data_sources table representing the many-to-many relationship between rule types and data sources.

Change Type

Mark the type of change your PR introduces:

  • 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

Manually tested that migrate up and migrate down work as expected.

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.

@blkt blkt self-assigned this Nov 20, 2024
@blkt blkt requested a review from a team as a code owner November 20, 2024 09:19
Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

gotta run make gen

@blkt blkt force-pushed the feat/migration-data-sources branch from fc09e7b to 1ba69a7 Compare November 20, 2024 09:26
@coveralls
Copy link

coveralls commented Nov 20, 2024

Coverage Status

coverage: 54.624% (-0.007%) from 54.631%
when pulling f8bdbdd on feat/migration-data-sources
into 3cc4ae8 on main.

JAORMX
JAORMX previously approved these changes Nov 20, 2024
Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@blkt
Copy link
Contributor Author

blkt commented Nov 20, 2024

@JAORMX I forgot to add timestamps, it should be good to go now.

@blkt blkt force-pushed the feat/migration-data-sources branch from 5b16d1b to 32e2828 Compare November 20, 2024 12:59
JAORMX
JAORMX previously approved these changes Nov 20, 2024
Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

A few nits about case insensitivity in names, as we don't want people to be able to do tricky things with datafetch and dataFetch doing different things.

database/migrations/000108_data_sources.up.sql Outdated Show resolved Hide resolved
database/migrations/000108_data_sources.up.sql Outdated Show resolved Hide resolved
Comment on lines +9 to +10
-- * functions can only reference one data source, and must be deleted
-- if the data source is deleted
Copy link
Member

Choose a reason for hiding this comment

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

I think I missed this in the design -- how do functions and data sources interact?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is that a data source is a "container" of functions that also defines a namespace.
As an example, this might be a shortened version of Trusty API v2 data source

version: v1
type: data-source
name: trusty
context:
  project: foo
rest:
 summary:
    endpoint: "https://api.trustypkg.dev/v2/summary?package_name={package_name}
    method: GET
    args:
      - package_name:
        schema:
          type: string
 pkg:
    endpoint: "https://api.trustypkg.dev/v2/pkg?package_name={package_name}"
    method: GET
    args:
      package_name:
        schema:
          type: string

This YAML defines data source trusty having "functions" summary and pkg. Eventually, they would be available to Rego engine as minder.datasource.trusty.summary(...) and minder.datasource.trusty.pkg(...). Some field names might be different, and some lists could be maps and vice versa, but that's about it.

Getting back to the comment, we thought that saving all that informatino in a single row of table data_sources was too coarse, and decided to add a separate table containing functions. I'm not sure I answered your question.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, the last time I'd reviewed the doc, multiple endpoints for the same data source weren't in it.

The tables make sense to me.

This migration adds support for three additional tables,
`data_sources` and `data_sources_functions` containing the definition
of a single data source in a many-to-one relationship, and a
`rule_type_data_sources` table representing the many-to-many
relationship between rule types and data sources.
@JAORMX JAORMX merged commit 64aec6a into main Nov 21, 2024
25 of 26 checks passed
@JAORMX JAORMX deleted the feat/migration-data-sources branch November 21, 2024 11:21
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.

5 participants