foundryvtt-docker
foundryvtt-docker copied to clipboard
Emit a friendly message when the installer type is wrong.
🗣 Description
See https://github.com/felddy/foundryvtt-docker/issues/282 for details. The short version is that it's a common error when using FOUNDRY_RELEASE_URL to download the windows installer. Previously a confusing message was emitted by the unzip command when that happened. This gives a message that should help beginners understand their mistake in choosing the wrong installer type.
💭 Motivation and Context
See https://github.com/felddy/foundryvtt-docker/issues/282
🧪 Testing
I haven't run this end-to-end at all yet, I just set up a little sandbox directory to test the case statement against some sample files.
✅ Checklist
- [x] This PR has an informative and human-readable title.
- [x] Changes are limited to a single goal - eschew scope creep!
- [x] All relevant type-of-change labels have been added.
- [x] I have read the CONTRIBUTING document.
- [ ] These code changes follow project standards.
- [ ] All relevant repo and/or project documentation has been updated to reflect the changes in this PR.
- [ ] Tests have been added to cover the changes in this PR.
- [ ] All new and existing tests pass.
From the end-to-end tests, it looks like this needs to be nested inside the check to see if the release-file exists, which makes sense.
I'm not sure what to make of that linter error. The subshell is already quoted and the variable is inside that. I feel like that's the idiomatic structure, but I'm not the strongest bashist in the world.
Let me know if you want me to take a pass at refining this, if it's close enough that you'd like to take it over the finish line yourself, or if it's not a contribution you're interested in.
Let me know if you want me to take a pass at refining this, if it's close enough that you'd like to take it over the finish line yourself, or if it's not a contribution you're interested in.
This is looking good, and think this is a great feature to add. I apologize for not responding earlier, I was weekending...
I think you've got the right idea moving the file check into the download block above. If you make that change, and approve the suggestion I made, I'll rerun the tests and we should be good to go.
I apologize for not responding earlier, I was weekending...
Definitely enjoy your time away from the keyboard, I'm certainly not here to apply time pressure to someone providing the public service of maintainership and you should handle things when it's convenient for you. I just wanted you to know I had looked at the tests, document what I was able to learn from them, and clarify that I'd take no further action without directional feedback.
I think you've got the right idea moving the file check into the download block above. If you make that change, and approve the suggestion I made, I'll rerun the tests and we should be good to go.
Done and done. Also merged develop in since this PR had fallen out of date. If the tests pass I think you should be good to merge. Thanks for the work you do on this image. I've worked with a lot of docker images and this is one is exceptionally pleasant to use.
I think you've got the right idea moving the file check into the download block above.
Erm... I didn't read this close enough and had put it in the installer block. Moved it up to the download block as requested.
NOW... pending passing tests it should be good to merge.
The build/test jobs failed again. I'm stumped this time, though. The log output is:
E Entrypoint | 2022-02-01 22:39:25 | [debug] Timezone set to: US/Eastern
E Entrypoint | 2022-02-01 22:39:25 | [info] Starting felddy/foundryvtt container v9.249.0
E Entrypoint | 2022-02-01 22:39:25 | [debug] CONTAINER_VERBOSE set. Debug logging enabled.
E Entrypoint | 2022-02-01 22:39:25 | [info] No Foundry Virtual Tabletop installation detected.
E Entrypoint | 2022-02-01 22:39:25 | [error] Unable to install Foundry Virtual Tabletop!
E Entrypoint | 2022-02-01 22:39:25 | [error] Either set set FOUNDRY_RELEASE_URL.
E Entrypoint | 2022-02-01 22:39:25 | [error] Or set FOUNDRY_USERNAME and FOUNDRY_PASSWORD.
E Entrypoint | 2022-02-01 22:39:25 | [error] Or set CONTAINER_CACHE to a directory containing foundryvtt-9.249.zip
But it's not clear to me how these changes would modify that behavior.
Weird. I would have expected that to work as well. I've got your fork in a branch here. I'll see if I can test it today and figure out what is going on.