Add support for downloading and installing informal modrinth packs #272

kMaiSmith · 2023-07-29T03:18:12Z

Backs #271

Purpose

Decouple the Modrinth .mrpack fetching mechanism from the pack installation mechanism to enable installing packs beyond the Modrinth API

Strategy

Extract all business logic captured in the InstallModrinthModpackCommand class into its own package, leaving the InstallModrinthModpackCommand a thin wrapper for the new package interface
Separate modpack fetching logic into the ModrinthAPIPackFetcher class backed by the IModrinthPackFetcher interface
Add ModrinthHttpPackFetcher to fetch non-Modrinth-API packs also implementing the IModrinthPackFetcher
Add ModrinthPackInstallerclass to capture.mrpack` extraction logic
Add new ModrinthModpack base class which is responsible for coordinating the fetch and install actions
Lots of tests

Checklist

Modrinth API modpack fetcher class fetches .mrpack from Modrinth API
Modrinth modpack installation/extraction class
Refactor InstallModrinthModpackCommand to use new logic
Add Http modpack fetcher
Migrate modpack code back to modrinth package

- Add additional test cases to ModrinthApiPackFetcher - ProjectRef knows how to extract project ID/version from URL

itzg · 2023-07-29T15:53:09Z

I'm sorry, these changes are way too disruptive. You'll need to be extremely minimal in your refactoring.

Specifically,

Do not create another package
Do not name things with "I" prefix. I dislike that convention and it is not a modern naming convention

kMaiSmith · 2023-07-29T17:12:11Z

I felt the need for restructure to get my head around the code for tests, but now with the tests i can comfortably undo many of code-wise refactors.

Do not create another package

I will re-merge these changes into the original package and see how i can minimize restructuring.

I will propose a class structure to minimize structural changes:

Utilize the InstallModrinthModpackCommand to act as the primary logic facilitator (in stead of the new ModrinthModpack)
Utilize the existing data fields on the InstallModrinthModpackCommand to pass around configurations to the fetcher and installer class
Move the ModrinthApiPackFetcher (+ Interface) and ModrinthPackInstaller into the modrinth package

Do not name things with "I" prefix

Noted. What are the modern practices for this? Would I just name the interface without the I?

I can tell I'm not quite getting the Reactive Streams just yet; I'm in the middle of giving the docs a read-through, and i would appreciate feedback on the way the ModrinthApiPackFetcher was put together especially in regards to the streams processing and Java best practices.

I expect I will be leaving the ModrinthPackInstaller logic mostly untouched as it should apply regardless of the source of the pack, but i do feel compelled to pursue extracting it so i can write some tests around it.

I appreciate and welcome any additional feedback, i do not aim to be disruptive. 😄

- Remove ModrinthModpack class and config - Use InstallModrinthModpackCommand as source of config

itzg · 2023-07-29T20:11:32Z

Sorry for my less-than-constructive initial feedback. It didn't help that I only had a few minutes at the time to reply via the mobile app. I also apologize for the total lack of existing unit tests for the Modrinth commands. I was going to suggest I write those before you proceed, but you beat me to it 😀. Please refer to src/test/java/me/itzg/helpers/fabric/FabricLauncherInstallerTest.java to see how I have been doing command/integration level unit tests without imposing extra design layers. Keep in mind, this is only a helper tool and over-design is a code smell in this case.

I prefer the sound of your revised strategy. In fact, I am expecting a majority or all of the changes to reside in InstallModrinthModpackCommand. Your goal is to introduce as few changes as possible to get from the picocli option for the mrpack file/URL and call the existing me.itzg.helpers.modrinth.InstallModrinthModpackCommand#processModpackZip.

As for avoiding "I" prefix. Yes, just name interfaces sensibly and intuitively. It shouldn't take a prefix to identify it's an interface...and in most cases the object oriented design should make interface/class non-important to the user/caller.

For the reactive calls, I recommend glancing through https://projectreactor.io/docs/core/release/reference/ and/or https://projectreactor.io/learn.

itzg

Just wanted to pass along a few initial code comments.

src/main/java/me/itzg/helpers/modrinth/ModrinthPackFetcher.java

src/main/java/me/itzg/helpers/modrinth/ModrinthApiPackFetcher.java

src/main/java/me/itzg/helpers/modrinth/ModrinthPackInstaller.java

- No longer passing InstallModrinthModpackCommand as config - Expect ModrinthApiClient to be passed in, vs constructed

- no longer consume InstallModrinthModpackCommand as config - add initial unit test demonstrating minimum assembly and extraction of modrinth pack

itzg · 2023-07-29T23:18:47Z

src/main/java/me/itzg/helpers/modrinth/ModrinthPackInstaller.java

+    private final boolean forceModloaderReinstall;
+    private final SharedFetchArgs sharedFetchArgs;
+
+    public ModrinthPackInstaller(ModrinthApiClient apiClient, SharedFetchArgs sharedFetchArgs, Path zipFile, Path outputDirectory, Path resultsFile, boolean forceModloaderReinstall) {


I was just thinking, rather pass in SharedFetchArgs it would be cleaner to pass in SharedFetch.Options.

This part confuses me: the only consumer of this in the whole class is the Quilt loader installer; why does the Quilt loader need it while Forge and Fabric don't? (I acknowledge this question is for my curiosity and desire for architectural consistency; having spent so much time in this code now, i want to come back to this class and clean it up but that falls beyond the scope of what i am trying to achieve here)

Fair question 🙂. I have been evolving the design over time, so Quilt is the newer of the three implementations with cleaner shared fetch usage. If you're interested in a little extra cleanup, the SharedFetch.Options should really be passed in rather than the default used here from the Options.builder().build() call.

mc-image-helper/src/main/java/me/itzg/helpers/fabric/FabricLauncherInstaller.java

Line 48 in 580e62b

try (SharedFetch sharedFetch = sharedFetch("fabric", Options.builder().build())) {

...and Forge installer is "pre shared fetch", so it's calling me.itzg.helpers.http.Fetch#fetch to create one-off instances.

itzg · 2023-07-29T23:19:24Z

The latest changes are looking really good. Thanks for jumping into this feature BTW.

- private functions in ModrinthModpackIndex use apiClient field

- ModrinthPackInstaller returns both Index and list of installed files (Required for generating manifest) - ModrinthTestHelpers.createModrinthPack returns byte array (Enables gluing whole test arch without writing tempfiles)

- Broken state, needs investigation

- cleanup test broken

itzg

Just taking a quick look at a recent change

src/test/java/me/itzg/helpers/modrinth/TestProjectRef.java

itzg · 2023-08-04T12:47:54Z

src/test/java/me/itzg/helpers/modrinth/TestProjectRef.java

+    void testProjectRefHasProjectSlug() {
+        projectRefUT = new ProjectRef(this.expectedSlug, null);
+
+        assertEquals(expectedSlug, projectRefUT.getIdOrSlug());


Please switch to https://assertj.github.io/doc/ rather than using the junit assertions. It'll be consistent with the rest of the project and they are much more expressive.

…acks

kMaiSmith · 2023-08-11T22:15:27Z

Just got it tested out and i was able to use it to download my self-hosted modpack. Due to the nature of my pack i would prefer not to share its URL for validation testing; if necessary I can assemble another one and share its URL with you.

What are the next steps for getting this merged?

itzg · 2023-08-12T04:25:40Z

It seems like unit tests will be sufficient, so don't worry about providing an additional mrpack URL for further manual testing.

From here the only remaining step is code review and iterating on changes from that.

itzg · 2023-08-12T14:34:44Z

Sorry, I needed to put a fix in for me.itzg.helpers.modrinth.InstallModrinthModpackCommand#extractOverrides. I can work on resolving the conflict in your branch if needed.

itzg

Looks great overall! Very nice refactorings. Please switch to AssertJ -- I provided a specific comment where it would have helped even more so.

src/main/java/me/itzg/helpers/modrinth/ModrinthPackInstaller.java

src/test/java/me/itzg/helpers/modrinth/InstallModrinthModpackCommandTest.java

- No longer uses args

kMaiSmith · 2023-08-14T16:26:17Z

I just took the body of the mainline extractOverrides method, everything is still functioning as i expected. AssertJ now proveds the assertions. ModrinthPackInstaller now consumes the Shared Fetch Options. I look forward to closing this out 😄

itzg

All looks great. I'll get this merged and cut a release. Don't worry if you find any follow up issues during final integration with the docker image repo -- can iterate on a follow up PR as needed.

itzg · 2023-08-14T18:13:36Z

Released in https://github.com/itzg/mc-image-helper/releases/tag/1.33.6

itzg · 2023-08-17T21:32:09Z

src/main/java/me/itzg/helpers/modrinth/InstallModrinthModpackCommand.java

-                        )
-                        .doOnNext(version -> log.debug("Resolved version={} from projectRef={}", version.getVersionNumber(), projectRef))
-                        .publishOn(Schedulers.boundedElastic()) // since next item does I/O
-                        .filter(version -> needsInstall(prevManifest, project, version))


@kMaiSmith I just realized you seem to have removed this "force synchronize" feature. Can you create a follow up PR to put it back?

Created #290 to track this

… modrinth packs (#272)" This reverts commit e5e6d46.

itzg · 2023-08-18T23:13:39Z

@kMaiSmith I needed to revert this PR in #292. It has caused a couple of regressions one of which is critical since it prevents users from restart their container with a previously installed modpack.

itzg · 2023-08-18T23:22:57Z

...I will revisit this over the weekend to see if I can fix the versionId being null in the resulting manifest.

drewgingerich · 2023-11-25T05:37:08Z

Hi there! I'm curious about the status of this PR. I'm looking to share an informal modrinth pack with friends by storing the .mrpack file in a GitHub repo, and then giving the URL to the minecraft docker container via env var to fetch and install. Going through a whole formal modpack release process on modrinth feels like overkill since I don't want to share it publicly.

As an aside, the existing feature to auto-fetch modpacks from modrinth is awesome. I'm coming back to minecraft after a few years and I'm amazed at how slick self-hosting a modded minecraft server is nowadays!

itzg · 2023-11-25T13:50:14Z

This MR is merged and ready to use in the image.

The only thing left to implement is handling local mrpack files, but your use case is covered with hosting in GitHub repo.

itzg · 2023-11-25T13:51:51Z

...looks like I need to fix the docs at https://docker-minecraft-server.readthedocs.io/en/latest/types-and-platforms/mod-platforms/modrinth-modpacks/#modpack-project

itzg · 2023-11-25T13:54:19Z

...and I misspoke, the final integration using mrpack URLs may not have been completed.

drewgingerich · 2023-11-27T06:20:14Z

Thanks for taking a look! I was wondering if it had slipped through the cracks since it was merged and then reverted, and GitHub doesn't really make that state visible.

itzg · 2023-11-27T14:24:51Z

FYI changes in this PR were restored in #293

kMaiSmith added 3 commits July 28, 2023 21:35

Add characterization tests to ProjectRef

1b8980c

Initial happy path test for downloading a modrinth pack with version id

60e84bc

Copy .mrpack extraction logic into ModrinthPackInstaller

ea4ca6c

- Add additional test cases to ModrinthApiPackFetcher - ProjectRef knows how to extract project ID/version from URL

Undo package separation of modrinth modpack functionality

a52f7cf

- Remove ModrinthModpack class and config - Use InstallModrinthModpackCommand as source of config

itzg reviewed Jul 29, 2023

View reviewed changes

src/main/java/me/itzg/helpers/modrinth/ModrinthPackInstaller.java Show resolved Hide resolved

itzg reviewed Jul 29, 2023

View reviewed changes

src/main/java/me/itzg/helpers/modrinth/ModrinthPackInstaller.java Outdated Show resolved Hide resolved

kMaiSmith added 3 commits July 29, 2023 17:35

ModrinthApiPackFetcher constructor takes arguments for fields

0cc41e8

- No longer passing InstallModrinthModpackCommand as config - Expect ModrinthApiClient to be passed in, vs constructed

ModrinthPackInstaller constructor takes fields as arguments

93d2c50

- no longer consume InstallModrinthModpackCommand as config - add initial unit test demonstrating minimum assembly and extraction of modrinth pack

Whitespacing cleanup

dcd9426

itzg reviewed Jul 29, 2023

View reviewed changes

kMaiSmith added 4 commits July 31, 2023 22:04

TestModrinthModpackInstaller proves the index is processed

950244d

- private functions in ModrinthModpackIndex use apiClient field

Break Modrinth test helper functions into test helper class

9df511e

Prove ModrinthPackInstaller downloads files for the modpack

9f05c84

kMaiSmith force-pushed the informal-modrinth-packs branch from 71260a9 to 51ea538 Compare August 1, 2023 03:23

kMaiSmith added 2 commits July 31, 2023 23:25

WIP: end-to-end characterization test of InstallModrinthModpackCommand

e96cdf4

- Broken state, needs investigation

First working characterization test of InstallModrinthModpackCommand

891ba70

itzg mentioned this pull request Aug 2, 2023

Modrinth modpack from local file itzg/docker-minecraft-server#2313

Closed

Add characterization test for manifest, cleanup

1970ab7

- cleanup test broken

itzg reviewed Aug 4, 2023

View reviewed changes

kMaiSmith added 5 commits August 4, 2023 23:00

Fix modrinth modpack cleanup test

e59b23f

InstallModrinthModpackCommand uses Fetcher and Installer classes

cfbc8e4

Add basic http fetcher using SharedFetch

b3f7b91

Test expects InstallModrinthModpackCommand to handle generic url modp…

a5de160

…acks

Rename Modrinth test classes to reflect rest of project

d9a56f9

Merge remote-tracking branch 'itzg/master' into informal-modrinth-packs

c016d27

kMaiSmith changed the title ~~WIP: Add support for downloading and installing informal modrinth packs~~ Add support for downloading and installing informal modrinth packs Aug 11, 2023

kMaiSmith marked this pull request as ready for review August 11, 2023 22:12

itzg self-requested a review August 12, 2023 17:48

itzg requested changes Aug 12, 2023

View reviewed changes

src/main/java/me/itzg/helpers/modrinth/ModrinthPackInstaller.java Outdated Show resolved Hide resolved

src/test/java/me/itzg/helpers/modrinth/InstallModrinthModpackCommandTest.java Outdated Show resolved Hide resolved

kMaiSmith added 3 commits August 14, 2023 11:52

Merge remote-tracking branch 'itzg/master' into informal-modrinth-packs

141739f

Switch assertions to AssertJ

ef82e8d

ModrinthPackInstaller uses Shared Fetch Options

4c02cc5

- No longer uses args

kMaiSmith requested a review from itzg August 14, 2023 16:46

itzg approved these changes Aug 14, 2023

View reviewed changes

itzg added the enhancement New feature or request label Aug 14, 2023

itzg merged commit e5e6d46 into itzg:master Aug 14, 2023
1 check passed

itzg reviewed Aug 17, 2023

View reviewed changes

itzg mentioned this pull request Aug 17, 2023

modrinth: force-sychronize needs to be re-implemented #290

Closed

itzg added a commit that referenced this pull request Aug 18, 2023

Revert "modrinth: Add support for downloading and installing informal…

c6ea460

… modrinth packs (#272)" This reverts commit e5e6d46.

itzg mentioned this pull request Aug 18, 2023

modrinth: reverting installing informal modrinth packs #292

Merged

itzg mentioned this pull request Aug 18, 2023

modrinth: resolve issue with null versionId in manifest itzg/docker-minecraft-server#2349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for downloading and installing informal modrinth packs #272

Add support for downloading and installing informal modrinth packs #272

kMaiSmith commented Jul 29, 2023 •

edited

Loading

itzg commented Jul 29, 2023

kMaiSmith commented Jul 29, 2023

itzg commented Jul 29, 2023 •

edited

Loading

itzg left a comment

itzg Jul 29, 2023

kMaiSmith Jul 30, 2023

itzg Jul 30, 2023

itzg Jul 30, 2023

itzg commented Jul 29, 2023

itzg left a comment

itzg Aug 4, 2023

kMaiSmith commented Aug 11, 2023

itzg commented Aug 12, 2023

itzg commented Aug 12, 2023

itzg left a comment

kMaiSmith commented Aug 14, 2023

itzg left a comment

itzg commented Aug 14, 2023

itzg Aug 17, 2023

itzg Aug 17, 2023

itzg commented Aug 18, 2023 •

edited

Loading

itzg commented Aug 18, 2023

drewgingerich commented Nov 25, 2023 •

edited

Loading

itzg commented Nov 25, 2023

itzg commented Nov 25, 2023

itzg commented Nov 25, 2023

drewgingerich commented Nov 27, 2023

itzg commented Nov 27, 2023

Add support for downloading and installing informal modrinth packs #272

Add support for downloading and installing informal modrinth packs #272

Conversation

kMaiSmith commented Jul 29, 2023 • edited Loading

Purpose

Strategy

Checklist

itzg commented Jul 29, 2023

kMaiSmith commented Jul 29, 2023

itzg commented Jul 29, 2023 • edited Loading

itzg left a comment

Choose a reason for hiding this comment

itzg Jul 29, 2023

Choose a reason for hiding this comment

kMaiSmith Jul 30, 2023

Choose a reason for hiding this comment

itzg Jul 30, 2023

Choose a reason for hiding this comment

itzg Jul 30, 2023

Choose a reason for hiding this comment

itzg commented Jul 29, 2023

itzg left a comment

Choose a reason for hiding this comment

itzg Aug 4, 2023

Choose a reason for hiding this comment

kMaiSmith commented Aug 11, 2023

itzg commented Aug 12, 2023

itzg commented Aug 12, 2023

itzg left a comment

Choose a reason for hiding this comment

kMaiSmith commented Aug 14, 2023

itzg left a comment

Choose a reason for hiding this comment

itzg commented Aug 14, 2023

itzg Aug 17, 2023

Choose a reason for hiding this comment

itzg Aug 17, 2023

Choose a reason for hiding this comment

itzg commented Aug 18, 2023 • edited Loading

itzg commented Aug 18, 2023

drewgingerich commented Nov 25, 2023 • edited Loading

itzg commented Nov 25, 2023

itzg commented Nov 25, 2023

itzg commented Nov 25, 2023

drewgingerich commented Nov 27, 2023

itzg commented Nov 27, 2023

kMaiSmith commented Jul 29, 2023 •

edited

Loading

itzg commented Jul 29, 2023 •

edited

Loading

itzg commented Aug 18, 2023 •

edited

Loading

drewgingerich commented Nov 25, 2023 •

edited

Loading