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

Implement database tests for subject table #88

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

rhmdnd
Copy link
Owner

@rhmdnd rhmdnd commented Jul 8, 2022

Fixes #81

@openshift-ci openshift-ci bot requested review from jhrozek and mrogers950 July 8, 2022 21:58
@openshift-ci openshift-ci bot added the approved label Jul 8, 2022
@rhmdnd rhmdnd force-pushed the add-subject-integration-tests branch 2 times, most recently from 050bdb4 to e8964b4 Compare July 12, 2022 16:02
@rhmdnd rhmdnd changed the title WIP: Implement database tests for subject table Implement database tests for subject table Jul 12, 2022
@rhmdnd rhmdnd force-pushed the add-subject-integration-tests branch 2 times, most recently from 98806f5 to 822be2a Compare July 12, 2022 16:43
Copy link
Collaborator

@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.

let's give this some thought... subjects are the entity that's being evaluated, right? If so... how will we divide OpenShift clusters, which have different attributes per machine pool? Will they have different subjects for each pool? Then, how will we add a relationship? e.g. worker nodes and control plane nodes belong to the same cluster.

@@ -1,5 +1,5 @@
CREATE TABLE IF NOT EXISTS subjects (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add documentation about what a Subject can be. I recall you mentioned that a subject may be the entity that's being audited for compliance. e.g. a server, a cluster, and so on. Let's mention this in documentation.

Copy link
Owner Author

Choose a reason for hiding this comment

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

FWIW - I do have an issue open to discuss the overall schema if we want to iron some of that out there (and take propose the outcome as documentation).

#84

@@ -1,5 +1,5 @@
CREATE TABLE IF NOT EXISTS subjects (
id serial PRIMARY KEY,
id uuid PRIMARY KEY,
name VARCHAR(255),
type VARCHAR(50)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about adding metadata to better help locate a subject? While we use very consistent naming for our servers (just from the name we'll know a lot about what server and where it is) some other places treat their servers more like cattle and have very vague names, which can get confusing. I'd suggest adding a json blob column that may store metadata for subjects.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah - that's a fair point. I also think metadata could be generalized and used across multiple tables. For example, I could see created_at, last_modified, and version as being valid metadata attributes for results and subjects. OSCAL uses a similar approach [0].

What are your thoughts on defining a schema for metadata, and reusing it across multiple entities (e.g., resources and subjects initially)? My concern with using a JSONB type for a column is that it promotes easy storage, but it can be more difficult to migrate those things out later. Long term, performance could be a concern using JSON over other types.

[0] #33

tests/integration/integration_test.go Show resolved Hide resolved
tests/integration/integration_test.go Show resolved Hide resolved
Make sure we test the current schema for the subject table. This commit
adds some basic tests that we can use as a template for testing
additional tables and schemas moving forward.

Fixes #81
@rhmdnd rhmdnd force-pushed the add-subject-integration-tests branch from 822be2a to 85da02e Compare July 13, 2022 19:07
Copy link
Collaborator

@Vincent056 Vincent056 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 14, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhmdnd, Vincent056

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot merged commit 31b86c1 into main Jul 14, 2022
@rhmdnd
Copy link
Owner Author

rhmdnd commented Jul 14, 2022

Following up that this will likely need more work and test coverage depending on the outcomes of #84

At the very least - this should serve as an example for other contributors to build more testing on as we develop more schemas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add database integration tests for subjects
3 participants