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

Add target field to modpack builds #438

Conversation

Indemnity83
Copy link
Contributor

Related to (and possibly closes) #269 and #398

This change adds a target field to mods in a modpack build. The target is done at the modpack build level instead of the mod itself because there may be cases where a mod might be universal in some packs, and client only in others (NEI is an example). There are three possible targets:

  • Universal: The mod is suitable for both client and server deployments (this is the default)
  • Client: The mod should be downloaded and used on the client only
  • Server: The mod should be downloaded and used on the server only

The API is also updated to allow queries for either client or server mod lists. The changes are backward compatible in that a query against /api/modpack/:slug/:build will return all client mods (mods targeted as universal or client). The changes to the API allow the addition of the target parameter (client or server) api/modpack/:slug/:build/:target. api/modpack/:slug/:build and api/modpack/:slug/:build/client return identical results.

TODO:

  • Changes to the build view template result in multiple change buttons, this was done to keep the merge simple and focused. Ideally a change should be implemented in the future to add an update button near the remove button (would be an extension of Save All Button #237) which updates both the version and the target.
  • Develop a tool which can take advantage of the added API to manage a server build too. I've started this, but haven't implemented anything yet (see issue Implement target in modpack get command Indemnity83/SolderToolbelt#1)

@GenPage
Copy link
Contributor

GenPage commented Sep 6, 2015

I pulled the test cases in first along with your other commits that didn't conflict.

Can you rebase and resubmit this one? Thanks!

@Indemnity83 Indemnity83 force-pushed the feature/add-build-modversion-target branch from 51c484b to f66c3c9 Compare September 6, 2015 06:00
@Indemnity83
Copy link
Contributor Author

Cleaned up the commits, should be a clean merge now.

@EntranceJew
Copy link
Contributor

I tested this out myself and it's great! I wrote a small script to utilize this and automate downloading the resources on my server and it works fairly well. 👍

@Indemnity83
Copy link
Contributor Author

Thanks for giving it a try!

Looking though this or again though I still see a ton of garbage commits. Is that just in my end or is it still messed up? I thought I had forced a push that would have cleaned it all up.

@EntranceJew
Copy link
Contributor

I'm not too experienced with that particular sort of thing, but following this doc minus the squashing seems like it should do it.

Of note, when I was merging the files from your feature branch into one of my branches there were some inconsistent indentations in the files but that might just be an artifact of the rebase not going right.

edit: I took a stab at it in my fork and it seems like everything worked out, but I had to ensure that my fork's dev and master were up to speed.

@GenPage
Copy link
Contributor

GenPage commented Sep 10, 2015

Hey @Indemnity83 and @EntranceJew

I'll take responsibility for this. Essentially the PR branch is behind dev still. I had a storm of TravisCI commits that really should of been done on a separate branch, squashed and PR'd, but it didnt happen as it was during a time sensitive issue with the Technic team

Can you rebase the PRs again? 😥

@Indemnity83 Indemnity83 force-pushed the feature/add-build-modversion-target branch from f66c3c9 to 58b6690 Compare September 11, 2015 02:45
@Indemnity83
Copy link
Contributor Author

Squashed and rebased

Requires migration.

This adds the ability to designate the target of particular mods in a build as being either client, server or both (universal).

Fix bug caused by removal of Minecraft MD5 hash
@Indemnity83 Indemnity83 force-pushed the feature/add-build-modversion-target branch from 58b6690 to dad2046 Compare September 11, 2015 03:48
@Indemnity83
Copy link
Contributor Author

Doh, knew I should have run the tests before pushing. 0a5a2c7 caused one of the new tests to fail, I've corrected that and re-squashed :)

@GenPage
Copy link
Contributor

GenPage commented Sep 22, 2015

Sorry its taken me so long to review. I really like the base of this. But before I merge the PR, I want to discuss this a bit more. There are a few things I would like to see before merging into the dev branch.

  • Maybe setup a seperate feature branch for this? since its a big change, and we can work on this without impacting other work
  • Mod versions should have a default target
    • This means that we will need to add functionality for this in the ModController
  • Adding an additional target, "Optional"
  • Target should be returned with the mods in the build, not just filtered in the API request
    • Ex.
{
  minecraft : "1.7.10",
  java: "1.7",
  memory: 1024,
  forge: null,
  mods: [ 
    {
      name: "testmod",
      version: "0.1",
      md5: "fb6582e4d9c9bc208181907ecc108eb1",
      url: "mods/testmod/testmod-0.1.zip"
      target: "universal"
    } 
  ],
}

Thoughts?

@Indemnity83
Copy link
Contributor Author

The reason that the API is filtered instead of just returning the target was so it wouldn't break the API. The launcher, and any other tool expects api/modpack/:slug/:build returns mods for the client. Maybe this was naive of the tools, but it is what it is at this point.

My thought was that the api/modpack/:slug/:build endpoint would be deprecated in favor of api/modpack/:slug/:build/client and api/modpack/:slug/:build/server and an optional flag would be added separately (this goes into tagging).

Adding a default target to the mod, 👍

Moving this into its own feature branch I'm indifferent to, either a feature branch on my fork, or a feature branch here its 6 of one and a half dozen of the other. In either place, the goal is to get it to a point where everybody is happy and it is merged into develop.

@GenPage GenPage mentioned this pull request Sep 22, 2015
…arget

Whitespace corrections for modpack target.
@Indemnity83
Copy link
Contributor Author

It doesn't look like this PR is going to be merged and in the interest of cleaning up my fork in prep for a v0.8 PR I'm moving this branch in my repository and closing the PR.

@Indemnity83 Indemnity83 closed this Mar 9, 2017
@Indemnity83 Indemnity83 deleted the feature/add-build-modversion-target branch March 9, 2017 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants