Switch sample-external-issuer to issuer-lib #42
Conversation
jetstack-bot
added
the
dco-signoff: yes
jetstack-bot
added
do-not-merge/work-in-progress
inteon
force-pushed
the
switch_to_issuer_lib
branch
from
d652587
to
8633767
Compare
inteon
force-pushed
the
switch_to_issuer_lib
branch
from
8633767
to
077e5ef
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
jetstack-bot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: Tim Ramlot <[email protected]>
inteon
force-pushed
the
switch_to_issuer_lib
branch
from
077e5ef
to
59235ef
Compare
cert-manager-prow
bot
removed
the
needs-rebase
Signed-off-by: Tim Ramlot <[email protected]>
Signed-off-by: Tim Ramlot <[email protected]>
cert-manager-prow
bot
removed
the
do-not-merge/work-in-progress
|
Huzzah! Tests now pass! Any updates needed to the README? I can take a look but probably not until this weekend at the earliest. Even if some changes are needed, I think might be worth merging this with documentation updates to follow, as in my opinion this is a better example as a standalone repository than trying to work from the example under issuer-lib itself. |
api/v1alpha1/clusterissuer_types.go
Outdated
| func (vi *ClusterIssuer) GetIssuerTypeIdentifier() string { | ||
| return "clusterissuers.sample-issuer.example.com" | ||
| } |
There was a problem hiding this comment.
suggestion: I think this function would benefit from comments encouraging users to change this value, right? This should be changed for every issuer based on this repo
api/v1alpha1/clusterissuer_types.go
Outdated
| return "clusterissuers.sample-issuer.example.com" | ||
| } | ||
|
|
||
| var _ v1alpha1.Issuer = &ClusterIssuer{} |
There was a problem hiding this comment.
suggestion: I think a lot of people using sample-external-issuer might be pretty new to Go based on what we've seen in the past. I like that we have an assertion here that ClusterIssuer satisfies the interface, but this Go idiom is pretty difficult to work out if you're new (and it's hard to Google it too).
Could we add a comment to make it clear why this is here? We don't need to go full handholding with this repo but I think this might be worth it!
api/v1alpha1/issuer_types.go
Outdated
| func (vi *Issuer) GetIssuerTypeIdentifier() string { | ||
| return "issuers.sample-issuer.example.com" | ||
| } | ||
|
|
||
| var _ v1alpha1.Issuer = &Issuer{} |
There was a problem hiding this comment.
suggestions: as above for the cluster scoped issuer!
internal/controllers/signer.go
Outdated
There was a problem hiding this comment.
suggestion: I think this file is missing function level comments. Given that this is the file people are likely to need to edit, I reckon comments explaining each function's purpose would be really helpful.
Obviously people can go to issuer-lib docs but having it here in the code is no bad thing!
Thank you for the feedback, I'll add the requested comments and update the README. |
…erIssuer Signed-off-by: Tim Ramlot <[email protected]>
Signed-off-by: Tim Ramlot <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SgtCoDFish The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cert-manager-prow
bot
added
the
approved
Replaces the hand-written issuer logic with the new issuer-lib.
Should drastically reduce the complexity of this sample issuer and thus simplify writing new issuers based on this sample issuer.