chore: move jwt generator #1501
Conversation
| @@ -0,0 +1,180 @@ | |||
| // Copyright 2024 Canonical. | |||
|
|
|||
| // jwtgenerator generates JWT tokens to authenticate | |||
There was a problem hiding this comment.
package godoc usually starts with Package <name>
|
|
||
| // jwtGeneratorDatabase specifies the database interface used by the | ||
| // JWT generator. | ||
| type jwtGeneratorDatabase interface { |
There was a problem hiding this comment.
i think these three interfaces should be exported as they are parameter types for the exported constructor
There was a problem hiding this comment.
also interface names do not need the jwt prefix now
There was a problem hiding this comment.
Will remove the jwt prefix, but why do they need to be exported? Maybe for a caller to verify that a struct satisfies the interface with a _ = jwtGeneratorDatabase but even that's a stretch because the caller can just call New() with whatever struct it has to satisfy the interface.
There was a problem hiding this comment.
but the thing is that you don't know what interface you need to satisfy without looking at the code since it is unexported. generally having exported methods with unexported argument types or returns is an anti-pattern
There was a problem hiding this comment.
but the thing is that you don't know what interface you need to satisfy without looking at the code since it is unexported
But even if it was exported, how would you know how to satisfy the interface without looking at the code?
| @@ -0,0 +1,357 @@ | |||
| // Copyright 2024 Canonical. | |||
| package jwtgenerator_test | |||
@alesstimec What aspect is repetitive? |
|
@alesstimec |
kian99
force-pushed
the
move-jwtgenerator
branch
from
d53de57 to
7869486
Compare
|
@kian99 |
|
|
||
| // generatorDatabase specifies the database interface used by the | ||
| // JWT generator. | ||
| type generatorDatabase interface { |
There was a problem hiding this comment.
do we want to go in this direction of having <package_name>Database? since it is unexported wouldn't be nice just to have database?
There was a problem hiding this comment.
I've changed it back to being exported. It could still be database but I don't mind much.
kian99
force-pushed
the
move-jwtgenerator
branch
from
a2f7b30 to
78482b7
Compare
Move the jwtGenerator into a separate package.
kian99
force-pushed
the
move-jwtgenerator
branch
from
78482b7 to
3782c49
Compare
Description
This PR is mostly a mechanical change to move the jwtGenerator into a separate package. It was already well structured enough to do this. I've also added a package doc and renamed the NewJWTGenerator function to simply
New.The package has been renamed to
jujuauth.Fixes JUJU-7272