foundryvtt-docker icon indicating copy to clipboard operation
foundryvtt-docker copied to clipboard

Emit a friendly message when the installer type is wrong.

Open mikelococo opened this issue 3 years ago • 6 comments

🗣 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.

mikelococo avatar Jan 30 '22 22:01 mikelococo

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.

mikelococo avatar Feb 01 '22 04:02 mikelococo

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.

felddy avatar Feb 01 '22 15:02 felddy

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.

mikelococo avatar Feb 02 '22 00:02 mikelococo

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.

mikelococo avatar Feb 02 '22 00:02 mikelococo

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.

mikelococo avatar Feb 02 '22 07:02 mikelococo

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.

felddy avatar Feb 02 '22 14:02 felddy