Skip to content

Refactor Issuer generation#432

Merged
leochr merged 1 commit intomainfrom
issuer-refactor
Nov 1, 2022
Merged

Refactor Issuer generation#432
leochr merged 1 commit intomainfrom
issuer-refactor

Conversation

@arturdzm
Copy link
Copy Markdown
Contributor

@arturdzm arturdzm commented Oct 27, 2022

No description provided.

Copy link
Copy Markdown
Member

@leochr leochr left a comment

Choose a reason for hiding this comment

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

@arturdzm Thanks for the PR. Look good. Just one question about error handling

Comment thread utils/reconciler.go
err = r.GenerateCMIssuer(bao.GetNamespace(), prefix, CACommonName, operatorName)
if err !=nil {
if errors.Is(err, APIVersionNotFoundError) {
return false, nil
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be return false, err ? similar to how Certificate is handled (here)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In the original code when certmanager.io API not available it didn't return an error so that reconciler goes on, either using other method (OCP) or using byo certificate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

@leochr leochr left a comment

Choose a reason for hiding this comment

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

@arturdzm Thanks for the clarification

@leochr leochr merged commit bbeb320 into main Nov 1, 2022
@leochr leochr deleted the issuer-refactor branch April 8, 2026 19:09
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.

2 participants