Skip to content
This repository has been archived by the owner on May 30, 2022. It is now read-only.

Store checks metadata in the DB instead of ARA #402

Merged

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Oct 28, 2021

Refactor the checks metadata catalog storage, to use the Database instead of ARA.
In order to do so:

  • We have a new table named checks, which stores the checks catalog data with the check ID, and the rest of the data as payload. I used this approach, as I expect changes in this structure, so having a json payload makes things easier to avoid migration issues, etc. The changes I expect might be like: add a new premium field, split the expected values to other column or table, etc. This is an open discussion: Do you prefer the usage of fixed table structure or json payload mode
  • Checks service code to get and store the metadata in the database
  • A new API call to post the metadata api/checks/catalog, (ApiCreateChecksCatalogtaHandler) to create the catalog data.
  • Update the runner to use the new post endpoint instead of ARA

PD: Maybe there are some ARA leftovers over there. I will remove everything once we have the checks posting finished too

@arbulu89 arbulu89 force-pushed the refactor/store-checks-metadata-db branch from c05cdcf to d190d7d Compare October 28, 2021 13:45
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Green light from my side 🟢 🚀

return nil, err
func (c *checksService) GetChecksCatalog() (models.ChecksCatalog, error) {
var checksRaw []*models.CheckRaw
result := c.db.Order("payload->>'name'").Find(&checksRaw)
Copy link
Member

Choose a reason for hiding this comment

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

ahah funny jsonb stuff. At least it works eheh. Will keep this in mind, thanks

Copy link
Member

Choose a reason for hiding this comment

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

lol at that font ligature ->>

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 is postgres specific unfortunately, but it works hehe

Copy link
Member

Choose a reason for hiding this comment

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

I feel we will need to soon to split entity and models, here it is done already with checkRaw being the persisted entity and check being the model

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to understand better your wording:

  • entity DB related struct
  • model API related struct
    Is this correct?
    Do we have some other internal structure to manage data better? I ask this, as we store a lot of data as jsonb which needs to be unmarshalled at some point.

@arbulu89 arbulu89 force-pushed the refactor/store-checks-metadata-db branch from e6fc429 to f1fea3e Compare October 29, 2021 09:24
Copy link
Member

@stefanotorresi stefanotorresi left a comment

Choose a reason for hiding this comment

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

This looks good, but I see a few problems coming up, which are not specific to this PR, but they are becoming more relevant as we add API endpoints.
The main problem I think is that we need a dedicated package for everything related to the web API, as it has its own data models and potentially will have its own business logic, which should be segregated from the web frontend part. This is not to be done now, of course, but let's keep this in mind.
As I mentioned below, for example, we are mixing models a little bit, and the distinction is not super clear to me.

web/checks_api.go Outdated Show resolved Hide resolved
web/checks_api.go Outdated Show resolved Hide resolved
web/checks_api.go Outdated Show resolved Hide resolved
web/checks_api.go Outdated Show resolved Hide resolved
web/checks_api_test.go Outdated Show resolved Hide resolved
web/checks_api.go Show resolved Hide resolved
web/app.go Outdated Show resolved Hide resolved
web/checks_api.go Outdated Show resolved Hide resolved
@arbulu89 arbulu89 force-pushed the refactor/store-checks-metadata-db branch from de2d85c to a41ba5d Compare October 29, 2021 13:30
@stefanotorresi stefanotorresi merged commit 9b8397d into trento-project:main Nov 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Improvement of existing features
Development

Successfully merging this pull request may close these issues.

4 participants