Skip to content

fix: OIDC issuerCA to load all certificates from PEM bundle#874

Open
JoaoBraveCoding wants to merge 2 commits intoobservatorium:mainfrom
JoaoBraveCoding:LOG-8727
Open

fix: OIDC issuerCA to load all certificates from PEM bundle#874
JoaoBraveCoding wants to merge 2 commits intoobservatorium:mainfrom
JoaoBraveCoding:LOG-8727

Conversation

@JoaoBraveCoding
Copy link
Copy Markdown
Contributor

@JoaoBraveCoding JoaoBraveCoding commented Feb 25, 2026

philipgough
philipgough previously approved these changes Feb 26, 2026
Copy link
Copy Markdown
Contributor

@philipgough philipgough left a comment

Choose a reason for hiding this comment

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

/lgtm

@squat any concerns?

@JoaoBraveCoding
Copy link
Copy Markdown
Contributor Author

Is there anything else I can help with to get this one merged?

Copy link
Copy Markdown
Member

@squat squat left a comment

Choose a reason for hiding this comment

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

I'm find with this functionality. I just have one style comment to improve clarity and prevent future mistakes with validation.

@JoaoBraveCoding
Copy link
Copy Markdown
Contributor Author

@squat thanks for the reply! However, I don't think your comment went through

for {
block, rest = pem.Decode(rest)
if block == nil {
if len(config.issuerCAs) == 0 {
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.

For style and clarity, let's instead break on line 107, ie, whenever block == nil since that's the exit condition and let's move this check outside the block, since that's the validation. I think that would be clearer IMO.

@squat
Copy link
Copy Markdown
Member

squat commented Mar 30, 2026

@JoaoBraveCoding oops. I just reposted!

Copy link
Copy Markdown
Member

@squat squat left a comment

Choose a reason for hiding this comment

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

Lgtm!

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.

3 participants