Added crafting tweaks functionality to vanillatweaks json method #1299
Conversation
itzg
left a comment
itzg
left a comment
There was a problem hiding this comment.
I know you had good intentions, but I am not happy with all of the whitespaces changes. I need some time to consider how to proceed unless you're willing to reverse those changes.
|
I don't really care if there's whitespace - it was done by a vim configuration I use which cleans up my code. Anyway, I reverted the whitespace changes (after 3 attempts, I need coffee). |
itzg
left a comment
itzg
left a comment
There was a problem hiding this comment.
Thanks for tightening up the ch ages. The feature looks great. I will also look at the test failure because my recent test script changes might have influenced this to fail without saying much
https://github.com/itzg/docker-minecraft-server/tree/master/tests/setuponlytests/vanillatweaks_file
shotah
left a comment
shotah
left a comment
There was a problem hiding this comment.
Looks great! can we get it added to testing and get the clean up added! Thanks again!
| ], | ||
| "items": ["armored elytra"] | ||
| }, | ||
| "craftingtweaks": { |
There was a problem hiding this comment.
Are you able to add this to the test file as well for integration testing?
./docker-minecraft-server/tests/setuponlytests/vanillatweaks_file/vanillatweaks-datapacks.json
| PACKS=$(jq -jc '.packs' $VANILLATWEAKS_FILE) | ||
| if [ ! "$PACKS" ]; then | ||
| log "ERROR: unable to retrieve packs from $VANILLATWEAKS_FILE" | ||
| CRAFTING_TWEAKS=$(jq -jc '.craftingtweaks' $VANILLATWEAKS_FILE) |
There was a problem hiding this comment.
Makes me want to rename PACKS. :) Thanks for adding this!
There was a problem hiding this comment.
FWIW a technique I have done for renaming container variables is like this
There was a problem hiding this comment.
I think this is just my bad choice of names. :) I short handed it, not realizing it was going to get expanded out. 👍
| if ! get -o $out_dir/crafting_tweaks.zip "https://vanillatweaks.net${DOWNLOAD_URL}"; then | ||
| log "ERROR: failed to download crafting tweaks from ${DOWNLOAD_URL}" | ||
| exit 2 | ||
| fi |
There was a problem hiding this comment.
Looks like cleanup is missing. Probably can be taken out of line 78-83 and made into its own follow up code block.
There was a problem hiding this comment.
No cleanup required, you just move the zip into the datapacks
|
@fleverest I found why this is failing, looks like some real bash commands were added to test.sh. In ci.yml I hard coded on line 39 to use 'sh', that needs to be changed to 'bash'. file: run: sh tests/setuponlytests/test.sh -> run: bash tests/setuponlytests/test.sh |
scripts/start-setupVanillaTweaks
Outdated
| fi | ||
|
|
||
| # Download and move crafting tweaks | ||
| if [[ "$CRAFTING_TWEAKS" ]] && [[ "$VT_VERSION" ]]; then |
There was a problem hiding this comment.
After looking at tests, this needs to also be checking for JQ's null string. "null". :(
Something like
if [[ "$CRAFTING_TWEAKS" ]] && [[ "$CRAFTING_TWEAKS" != "null" ]] && [[ "$VT_VERSION" ]]; thenThere was a problem hiding this comment.
I'll add this now
There was a problem hiding this comment.
Added it for the $PACKS variable too.
| fi | ||
|
|
||
| mkdir -p "$out_dir" | ||
| if ! get -o $out_dir/crafting_tweaks.zip "https://vanillatweaks.net${DOWNLOAD_URL}"; then |
There was a problem hiding this comment.
This is a zip of zips. Does this not need to be unzipped?
There was a problem hiding this comment.
Works fine locally 🤷 I checked the file downloaded and it's a single zipped directory, unlike the datapacks zip.
There was a problem hiding this comment.
Agreed, I did end up looking and that is a single zip. My apologies, thought it was like datapacks.
|
Ok, I think that I addressed everything - let me know if you need anything else from me. |
| # Download and move crafting tweaks | ||
| if [[ "$CRAFTING_TWEAKS" ]] && [[ "$CRAFTING_TWEAKS" != "null"]] && [[ "$VT_VERSION" ]]; then | ||
| VT_ZIPDATA_URL=https://vanillatweaks.net/assets/server/zipcraftingtweaks.php | ||
| DOWNLOAD_URL=$(curl -X POST -F "packs=${CRAFTING_TWEAKS}" -F "version=${VT_VERSION}" $VT_ZIPDATA_URL | jq -r '.link') |
There was a problem hiding this comment.
FYI realized I need to add POST support in mc-image-helper
|
|
||
| # Download and unzip packs | ||
| if [[ "$PACKS" ]] && [[ "$VT_VERSION" ]]; then | ||
| if [[ "$PACKS" ]] && [[ "$PACKS" != "null"]] && [[ "$VT_VERSION" ]]; then |
There was a problem hiding this comment.
I'm not sure offhand why test revealed a bash error here. The syntax seems to be defensive. I'll help investigate this too.
There was a problem hiding this comment.
We can also use //empty in the jq like I did for the forge API. Then it'll return empty array like we'd expect. 🤔
current_file_name=$(jq -n "$current_project" | jq -jc '.fileName // empty' )
There was a problem hiding this comment.
It just dawned on me that the failing test is the full startup one that only sets EULA and VERSION. On master it's
Now I'm wondering why the VANILLATWEAKS logic is even getting hit here.
|
@fleverest some of this may be fixed by just fetching from up stream. I'm happy to jump in and get this over the line if you need any help. I see your branch is behind.
|
|
@itzg are we able to close this PR now that we have one coming in for Crafting, Resource, and Datapacks? |
|
Sure. #1336 is the new PR and @fleverest I'll make sure to add you as co-author the merge. Fee free to add comments over there too. |
3 participants
I altered the error messages to specify whether any error occurred during loading the datapacks or crafting tweaks. Also I removed a heap of trailing whitespace from the README.