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

Code Cleanup Around Audit Backends #20933

Merged
merged 4 commits into from
Jun 5, 2023
Merged

Conversation

marcboudreau
Copy link
Contributor

This PR cleans up an unused field in the backendEntry struct and removes function arguments that were only needed to initialize that unnecessary field in the struct.

As an added bonus, the few compiler warnings that were reported in the audit_test.go file were also resolved.

Marc Boudreau added 2 commits June 1, 2023 11:35
Copy link

@peteski22 peteski22 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 not certain on what the contents of the changelog entry should be (if it needs to refer to audit specifically, or if its changelog worthy if its 'clean up'), but LGTM. 😄

@@ -239,7 +239,7 @@ func TestCore_DisableAudit(t *testing.T) {
c, keys, _ := TestCoreUnsealed(t)
c.auditBackends["noop"] = corehelpers.NoopAuditFactory(nil)

existed, err := c.disableAudit(namespace.RootContext(nil), "foo", true)
existed, err := c.disableAudit(namespace.RootContext(context.TODO()), "foo", true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we're using context.TODO() over context.Background()? I think Background fits better for this purpose, as we know we should be using a context and the function does properly accept one. I could be missing something, though :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

context.TODO() was the suggested replacement in the go-staticcheck warning. But I can give context.Background() a try as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

context.TODO() was the suggested replacement in the go-staticcheck warning. But I can give context.Background() a try as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've replaced context.TODO() with context.Background() and all of the tests continue passing, so I'm in favour of using the Background function over the TODO function, especially since I noticed afterwards, other tests were also using the Background function when a Context was needed.

Copy link
Contributor

@VioletHynes VioletHynes left a comment

Choose a reason for hiding this comment

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

Looks great! Simplifying function arguments is always a good thing!

@marcboudreau
Copy link
Contributor Author

I'm not certain on what the contents of the changelog entry should be (if it needs to refer to audit specifically, or if its changelog worthy if its 'clean up'), but LGTM. 😄

@peteski22 do you mean changing improvement for clean up in the change log file?

@peteski22
Copy link

I'm not certain on what the contents of the changelog entry should be (if it needs to refer to audit specifically, or if its changelog worthy if its 'clean up'), but LGTM. 😄

@peteski22 do you mean changing improvement for clean up in the change log file?

No, I just wasn't sure if a changelog is required, or what section things need to go under for clean up in general. 😄

@marcboudreau marcboudreau merged commit 4374d76 into main Jun 5, 2023
@marcboudreau marcboudreau deleted the marcboudreau/audit-code-cleanup branch June 5, 2023 20:53
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.

3 participants