Skip to content

fix #64 adds settable check functions for cert/serverCert #65

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Apr 10, 2025

Conversation

andrewpmartinez
Copy link
Member

@andrewpmartinez andrewpmartinez commented Apr 10, 2025

Done to support openziti/ziti#2984

@andrewpmartinez andrewpmartinez requested a review from a team as a code owner April 10, 2025 13:41
@@ -76,9 +76,15 @@ type Identity interface {
// StopWatchingFiles reversed WatchFiles.
StopWatchingFiles()

// IsCertSettable returns nil if the "cert" certificate storage supports writing, used before calling SetCert()
IsCertSettable() error
Copy link
Member

Choose a reason for hiding this comment

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

imo, it seems strange for an 'Is' function to return error not a bool

Copy link
Member Author

@andrewpmartinez andrewpmartinez Apr 10, 2025

Choose a reason for hiding this comment

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

It is a go thing because errors are returned, not raised.

Consider this set of outcomes: for (bool,error) function return values:

1: true, <nil>
2: false, err
3: false, <nil>

It forces the caller to inspect two values instead of 1. By returning error only, they only have to inspect 1. Additionally, nothing stops the code from returning true, error and in which case, what does that mean?

Returning just bool doesn't allow error propagation for other issues.

Copy link
Member

Choose a reason for hiding this comment

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

:D if you say so. I'd just prefer the "Is" to be "Verify" or "Check" or literally anything that isn't "Is" but i don't care all that much

- workflows updated to ref go.mod for go version and update modules used
- go 1.23 selected as it is the minimum version currently supported w/o
  compat issues with the SDK
Copy link
Member

@plorenz plorenz left a comment

Choose a reason for hiding this comment

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

some linter errors, but otherwise looks good

@andrewpmartinez andrewpmartinez merged commit c351318 into main Apr 10, 2025
6 checks passed
@andrewpmartinez andrewpmartinez deleted the fix.64.add.cert.serverCert.isSettable.funcs branch April 10, 2025 14:39
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