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

Purge logs cmd #1008

Merged
merged 18 commits into from
Jul 26, 2023
Merged

Purge logs cmd #1008

merged 18 commits into from
Jul 26, 2023

Conversation

mina1460
Copy link
Contributor

Description

Adds a command and an endpoint in juju that purges logs before a certain date

Fixes CSS-3781

Engineering checklist

Check only items that apply

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

Test instructions

not yet tested

@mina1460 mina1460 changed the base branch from main to feature-rebac July 20, 2023 09:07
@mina1460 mina1460 requested review from kian99 and babakks July 20, 2023 09:58
Copy link
Contributor

@kian99 kian99 left a comment

Choose a reason for hiding this comment

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

lgtm with some comments

cmd/jimmctl/cmd/purgelogs_test.go Outdated Show resolved Hide resolved
internal/db/applicationoffer.go Outdated Show resolved Hide resolved
internal/jimm/purgeLogs.go Outdated Show resolved Hide resolved
@kian99
Copy link
Contributor

kian99 commented Jul 20, 2023

I made my comments and forgot to click submit. I see some new commits came through since, will take another look in a bit

.gitignore Show resolved Hide resolved
@@ -79,6 +80,7 @@ func init() {
r.AddMethod("JIMM", 4, "UpdateMigratedModel", updateMigratedModelMethod)
r.AddMethod("JIMM", 4, "AddCloudToController", addCloudToControllerMethod)
r.AddMethod("JIMM", 4, "RemoveCloudFromController", removeCloudFromControllerMethod)
r.AddMethod("JIMM", 4, "PurgeLogs", purgeLogsMethod)
Copy link
Contributor

Choose a reason for hiding this comment

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

not for this PR but for future consideration. Imagine we had clients out there that were already using facade version 4. .then we release these changes and release a new client that is also using client facade version 4.. then they try to use this client to interact with a server that has not been upgraded to include the new method yet - we'd have a problem :)

internal/jujuapi/jimm.go Show resolved Hide resolved
api/params/params.go Show resolved Hide resolved
api/params/params.go Show resolved Hide resolved
cmd/jimmctl/cmd/purgelogs.go Outdated Show resolved Hide resolved
cmd/jimmctl/cmd/purgelogs.go Outdated Show resolved Hide resolved
internal/db/applicationoffer.go Outdated Show resolved Hide resolved
internal/db/applicationoffer.go Outdated Show resolved Hide resolved
internal/jimm/purgeLogs.go Outdated Show resolved Hide resolved
Signed-off-by: Mina Ashraf <mina.gamil@canonical.com>
api/params/params.go Show resolved Hide resolved
api/params/params.go Show resolved Hide resolved
cmd/jimmctl/cmd/purgelogs.go Outdated Show resolved Hide resolved
cmd/jimmctl/cmd/export_test.go Outdated Show resolved Hide resolved
cmd/jimmctl/cmd/export_test.go Outdated Show resolved Hide resolved
cmd/jimmctl/cmd/purgelogs_test.go Outdated Show resolved Hide resolved
cmd/jimmctl/cmd/purgelogs_test.go Outdated Show resolved Hide resolved
cmd/jimmctl/cmd/purgelogs.go Outdated Show resolved Hide resolved
cmd/jimmctl/cmd/purgelogs.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kian99 kian99 left a comment

Choose a reason for hiding this comment

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

I'm happy with this now, provided Ales gives the thumbs up.

tomorrow := relativeNow.AddDate(0, 0, 1)
deleted_count, err := s.Database.DeleteAuditLogsBefore(ctx, tomorrow.Format(time.RFC3339))
// check that logs have been deleted
c.Assert(err, qt.IsNil)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add an assertion that the count of logs in the DB is 1. But not super necessary.

cmd/jimmctl/cmd/purgelogs_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alesstimec alesstimec left a comment

Choose a reason for hiding this comment

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

LGTM modulo some comments

cmd/jimmctl/cmd/purgelogs.go Outdated Show resolved Hide resolved
cmd/jimmctl/cmd/purgelogs.go Outdated Show resolved Hide resolved
cmd/jimmctl/cmd/purgelogs.go Outdated Show resolved Hide resolved
cmd/jimmctl/cmd/purgelogs.go Outdated Show resolved Hide resolved
Signed-off-by: Mina Ashraf <mina.gamil@canonical.com>
@kian99 kian99 mentioned this pull request Jul 25, 2023
3 tasks
internal/db/audit.go Outdated Show resolved Hide resolved
internal/jimm/purgeLogs.go Outdated Show resolved Hide resolved
cmd/jimmctl/cmd/purgelogs.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ale8k ale8k left a comment

Choose a reason for hiding this comment

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

lgtm bar mini comments, will merge mine in a moment as it's updated to reflect yours

@mina1460 mina1460 merged commit fe55d77 into canonical:feature-rebac Jul 26, 2023
1 check 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.

4 participants