feat: refactor auth token#105
feat: refactor auth token#105helayoty merged 1 commit intoAzure:mainfrom helayoty:refactor-auth-token
Conversation
helayoty
requested review from
Arvindthiru,
minhng22 and
ryanzhang-oss
and removed request for
ryanzhang-oss
| FetchToken(ctx context.Context) (AuthToken, error) | ||
| } | ||
|
|
||
| type AuthTokenWriter interface { |
There was a problem hiding this comment.
why do we need a token writer interface? Is it for testing? There is no multiple implementations of this interface for the foreseeable future either.
There was a problem hiding this comment.
For unit testing.
There was a problem hiding this comment.
I wonder how does this interface make the code more testable?
There was a problem hiding this comment.
Because it allows us to write to memory instead of the file system.
pkg/authtoken/token_refresher.go
Outdated
| refreshCalculate func(interfaces.AuthToken) time.Duration | ||
| afterFunc func(time.Duration, func()) *time.Timer |
There was a problem hiding this comment.
is there any reason to make these functions? For test?
There was a problem hiding this comment.
To be able to test it.
There was a problem hiding this comment.
can they not just be a function instead? I don't get why it has to be a function pointer.
There was a problem hiding this comment.
Both are functions not pointers to functions.
You might have gotten confused at afterFunc which returns a pointer to a time.Time (in order to match time.AfterFunc's signature)
|
Good job refactoring the code. It looks much better now. |
Codecov Report
@@ Coverage Diff @@
## main #105 +/- ##
=======================================
Coverage ? 66.82%
=======================================
Files ? 3
Lines ? 615
Branches ? 0
=======================================
Hits ? 411
Misses ? 197
Partials ? 7 Continue to review full report at Codecov.
|
ryanzhang-oss
left a comment
ryanzhang-oss
left a comment
There was a problem hiding this comment.
there is a common patten on how to use timer
) Bumps [step-security/harden-runner](https://github.com/step-security/harden-runner) from 2.12.0 to 2.12.1. - [Release notes](https://github.com/step-security/harden-runner/releases) - [Commits](step-security/harden-runner@0634a26...002fdce) --- updated-dependencies: - dependency-name: step-security/harden-runner dependency-version: 2.12.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Britania Rodriguez Reyes <145056127+britaniar@users.noreply.github.com>
3 participants
Description of your changes
Refactor configprovider to be "authtoken"
Fixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer
Design Doc: https://msazure.visualstudio.com/CloudNativeCompute/_git/CloudNativeCompute.wiki/pullrequest/6139435?path=/AKS/Team-Logistics/Caravel/SIG-Multi%252DCluster/Upstream/Join-%252D-Leave-workflow/Decision-Records/init-sidecar-containers.md