Skip to content
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

Update storage.go #21

Merged
merged 2 commits into from
Mar 24, 2022
Merged

Update storage.go #21

merged 2 commits into from
Mar 24, 2022

Conversation

mholt
Copy link

@mholt mholt commented Mar 8, 2022

Recent changes (not yet tagged) in CertMagic add context.Context to the Storage interface methods, as well as make use of the new fs.ErrNotExist error value.

Because it's a breaking change in CertMagic (which is not yet a stable 1.0), I'm submitting this PR as a courtesy. It's a quick edit I made manually in the browser, so it may need a little massaging if I missed something. Let me know if you have any questions!

(I only added context to your function signatures so they would be compatible with the updated interface. You might want to consider actually using the context for cancellation, if possible, when you get a chance. Thanks!)

@pteich
Copy link
Owner

pteich commented Mar 10, 2022

Thanks @mholt I'll have a look at it and get back to you if I need some clarification.

@mholt
Copy link
Author

mholt commented Mar 24, 2022

Hey @pteich I think I might have the updated API go out with the 2.5 release of Caddy (and the 0.16 release of CertMagic). Anything you need from me to get this merged?

@pteich pteich merged commit 106466b into pteich:master Mar 24, 2022
@pteich
Copy link
Owner

pteich commented Mar 24, 2022

@mholt I'm sorry I forgot about that with all the daily business. I's merged now and I'll take care of proper context usage in a follow up. Thanks again for your work and the ping. :)

@mholt
Copy link
Author

mholt commented Mar 24, 2022

No worries, I know how it goes! Thanks! Your plugin is decently popular so I wanted to ensure we didn't leave it behind.

@pteich
Copy link
Owner

pteich commented Mar 25, 2022

@mholt Are the changes regarding CertMagic already in the Caddy master branch? Because I'm getting messages like this when I use go get -u on the Caddy dependency:

../../go/pkg/mod/github.com/caddyserver/caddy/v2@v2.5.0-beta.1.0.20220325045403-a58f240d3ecb/caddy.go:639:27: not enough arguments in call to certmagic.CleanUpOwnLocks
        have (*zap.Logger)
        want (context.Context, *zap.Logger)
../../go/pkg/mod/github.com/caddyserver/caddy/v2@v2.5.0-beta.1.0.20220325045403-a58f240d3ecb/sigtrap_posix.go:39:30: not enough arguments in call to certmagic.CleanUpOwnLocks
        have (*zap.Logger)
        want (context.Context, *zap.Logger)

@mholt
Copy link
Author

mholt commented Mar 25, 2022

Not yet, I will probably be doing that soon (today or early next week)

@mholt
Copy link
Author

mholt commented Mar 25, 2022

@pteich Alright, I went ahead and updated CertMagic in Caddy and it should compile for you now: caddyserver/caddy@d06d0e7

@pteich
Copy link
Owner

pteich commented Mar 25, 2022

Works like a charm now! Thanks @mholt

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