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

caddytest.Tester should ensure that AppDataDir exists #5974

Closed
tgeoghegan opened this issue Dec 12, 2023 · 5 comments · Fixed by #5976
Closed

caddytest.Tester should ensure that AppDataDir exists #5974

tgeoghegan opened this issue Dec 12, 2023 · 5 comments · Fixed by #5976
Labels
feature ⚙️ New feature or request good first issue 🐤 Good for newcomers help wanted 🆘 Extra attention is needed

Comments

@tgeoghegan
Copy link
Contributor

Caddy (including Caddies run as caddytest.Tester) will create instance.uuid if it doesn't already exist. However, this will fail if caddy.AppDataDir() doesn't already exist, which appears to be the case when caddytest.Tester is used on Linux (though not on Windows or macOS, which I don't understand).

This makes it difficult to use caddytest.Tester to test modules that need caddy.InstanceID() to work. See for example my struggles in mholt/caddy-ratelimit#34, where I resorted to doing os.MkdirAll(caddy.AppDataDir()), which works well enough in CI, but doesn't feel like a great solution.

It'd be nice if caddytest.Tester could consistently guarantee that AppDataDir will exist, or perhaps it could avoid writing the instance ID to disk altogether since a caddytest.Tester instance is not likely to need persistence across restarts.

@mohammed90 mohammed90 added help wanted 🆘 Extra attention is needed feature ⚙️ New feature or request good first issue 🐤 Good for newcomers labels Dec 12, 2023
@mholt
Copy link
Member

mholt commented Dec 12, 2023

Good point. Redoing our caddytest package was on my list for 2.8 (then the baby came early, then life got busy, and I got behind on my backlog, so testing has been deferred another cycle again, sorry).

But in the meantime we could probably work in a fix for this before then.

@armadi1809
Copy link
Contributor

armadi1809 commented Dec 12, 2023

I can take a look at this!

@mholt
Copy link
Member

mholt commented Dec 12, 2023

Sounds good, let us know if you have questions @armadi1809

@armadi1809
Copy link
Contributor

@mholt I guess this can be resolved in two ways:

1- Inside the InstanceID function, check if the path returned by AppDataDir() exists and create it if it does not.
2- get rid the part of the InstanceID function that writes the UUID to disk.

Which one is preferred? After a quick search through the code base, it doesn't look like the UUID file is being used somewhere else other than where it's being created (i.e. inside the InstanceID function) but I might be wrong.

@mholt
Copy link
Member

mholt commented Dec 13, 2023

@armadi1809 I think 1 is best. A call to MkdirAll() should do the trick.

@tgeoghegan AppDataDir() honors XDG env vars, so maybe tests could set XDG_DATA_HOME to a temporary directory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request good first issue 🐤 Good for newcomers help wanted 🆘 Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants