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

fix(cleanup): ensure all methods register their cleanup schedules #1337

Merged
merged 4 commits into from
Feb 14, 2023

Conversation

GeorgeMac
Copy link
Contributor

@GeorgeMac GeorgeMac commented Feb 14, 2023

It was observed that OIDC method-created authentications were not getting cleared away.
The cleanup process when created was hardcoded to only perform cleanup for METHOD_TOKEN (mistakenly).

This was an easy mistake (not registering the method in all the places). It was why I added the generic method container and the AllMethods() support to ensure that adding new methods was as simple as ensuring the methods returns it info here:

return []StaticAuthenticationMethodInfo{
a.Token.Info(),
a.OIDC.Info(),
}

The types should depend on performing background operations of this AllMethods() list. Instead of having to manually add new methods everywhere they're created.

However, I never updated cleanup to take advantage of this.
This change does that. It updates the cleanup process to walk this list and create a cleanup goroutine per method.

Additionally, the cleanup test ensures that each method defined on all methods is configure and tested.

@GeorgeMac GeorgeMac requested a review from a team as a code owner February 14, 2023 12:07
@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2023

Codecov Report

Merging #1337 (3cda11e) into main (6144f32) will decrease coverage by 0.35%.
The diff coverage is 37.50%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1337      +/-   ##
==========================================
- Coverage   80.81%   80.47%   -0.35%     
==========================================
  Files          43       43              
  Lines        3336     3344       +8     
==========================================
- Hits         2696     2691       -5     
- Misses        512      523      +11     
- Partials      128      130       +2     
Impacted Files Coverage Δ
internal/config/authentication.go 76.66% <23.07%> (-6.97%) ⬇️
internal/cleanup/cleanup.go 71.66% <54.54%> (-5.76%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

couple minor suggestions. but otherwise lgtm.

I think we should prob create a release/1.18 branch and rebase this onto the v1.18.1 tag in order to get this out in v1.18.2 today.

internal/cleanup/cleanup.go Outdated Show resolved Hide resolved
internal/config/authentication.go Outdated Show resolved Hide resolved
internal/config/authentication.go Outdated Show resolved Hide resolved
@GeorgeMac
Copy link
Contributor Author

GeorgeMac commented Feb 14, 2023

Agreed @markphelps 👍 (on all counts)

I adjusted the PR based on your feedback.

@GeorgeMac GeorgeMac merged commit 1bd9924 into main Feb 14, 2023
@GeorgeMac GeorgeMac deleted the gm/cleanup-all-method-schedules branch February 14, 2023 14:23
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