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

OSIDB-2269: implement history on Flaw, Affects and FlawMeta #483

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JimFuller-RedHat
Copy link
Contributor

@JimFuller-RedHat JimFuller-RedHat commented Mar 25, 2024

This PR implements the audit history tracking feature ... and surfaces up a history key on flaws, affects and flawmeta in REST API.

By default history is not included and one must use the include_history url param to enable:

osidb/api/v1/flaws?include_history=True

In addition added a primitive audit endpoint which lists all events:

osidb/api/v1/audit

NOTE - The next PR will contain django migration for batch loading all BZ historical data into these new event tables as well as remove some older event related cruft. There is no point running this migration in stage (as stage has no data).

@JimFuller-RedHat JimFuller-RedHat self-assigned this Mar 25, 2024
@JimFuller-RedHat JimFuller-RedHat marked this pull request as draft March 25, 2024 14:03
@JimFuller-RedHat JimFuller-RedHat force-pushed the audit-history-feature branch 2 times, most recently from 4b4b479 to 9161aa7 Compare March 25, 2024 18:47
@JimFuller-RedHat JimFuller-RedHat force-pushed the audit-history-feature branch 10 times, most recently from 4f9cfbc to 9aee160 Compare March 26, 2024 20:57
@JimFuller-RedHat JimFuller-RedHat marked this pull request as ready for review March 26, 2024 21:23
@JimFuller-RedHat JimFuller-RedHat force-pushed the audit-history-feature branch 11 times, most recently from 4967e49 to 2e18343 Compare March 28, 2024 12:04
@JimFuller-RedHat JimFuller-RedHat force-pushed the audit-history-feature branch 13 times, most recently from b73d2e2 to f69d318 Compare March 29, 2024 08:29
Copy link
Contributor

@osoukup osoukup left a comment

Choose a reason for hiding this comment

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

I am submitting a partial review. Will continue tomorrow.

osidb/migrations/0123_history_audit.py Show resolved Hide resolved
pghistory.InsertEvent(),
pghistory.UpdateEvent(),
pghistory.DeleteEvent(),
exclude="local_updated_dt,meta_attr,_alerts",
Copy link
Contributor

Choose a reason for hiding this comment

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

The _alerts attribute will unfortunately soon cause some conflict as Daniel is just reworking it into a proper model. That future model can also be excluded from the history as we can recreate the alerts if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah thx for the headsup!

pghistory.DeleteEvent(),
exclude="meta_attr,_alerts",
model_name="FlawMetaAudit",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

FlawMeta is a relic model and a subject for a removal (with the next major release). All of its parts were already migrated into other places (like FlawReference etc.). You can simply exclude it from the audit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya I wondered, thx ... will remove ... I will reach out to u to discuss if any other models need the treatment

Copy link
Contributor

@osoukup osoukup left a comment

Choose a reason for hiding this comment

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

Submitting the second part of my review.

@@ -890,6 +972,7 @@ class Meta:
+ ACLMixinSerializer.Meta.fields
+ AlertMixinSerializer.Meta.fields
+ TrackingMixinSerializer.Meta.fields
+ HistoryMixinSerializer.Meta.fields
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: I am sure that it is not always possible but when it is I would prefer ordering the list-like structures alphabetically so a human can easily see what is there and what is not

def test_flawevent(self):
""" """
# create a flaw with an affect
Flaw.objects.all().delete()
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why the flaws are deleted first. Looking at the event count later it seems that it generates no event so I guess it has no effect - which is what I would assume at the beginning of a test (no flaws present).

Copy link
Contributor

@osoukup osoukup left a comment

Choose a reason for hiding this comment

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

So overall this LGTM with one exception which is FlawMeta. It should be removed soon so we should not add more code using it so the removal is not getting complicated. Otherwise a lot of great work done!

Copy link
Contributor

@JakubFrejlach JakubFrejlach left a comment

Choose a reason for hiding this comment

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

I have same smaller remarks or question but otherwise looking good!

include_history_param = None
# Get include history from request
if request:
include_history_param = request.query_params.get("include_history")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: You might want to describe this query parameter in OpenAPI schema the same way as we did with the include_fields or exclude_fields, see api_views.py for that.

Comment on lines +460 to +461
if include_history_param is None:
self.fields.pop("history", None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting basically include_fields=<anything> will surface the history?

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.

None yet

3 participants