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

Implemented ViaProxy bootstrap #4201

Merged
merged 35 commits into from
Feb 19, 2024

Conversation

RaphiMC
Copy link
Contributor

@RaphiMC RaphiMC commented Oct 8, 2023

Implemented a bootstrap for ViaProxy. This will replace the ViaProxyGeyserPlugin.

@rtm516 rtm516 added the PR: Feature When a PR implements a new feature label Oct 8, 2023
@Kas-tle
Copy link
Member

Kas-tle commented Oct 20, 2023

Also please add viaproxy to be archived as build artifacts by the pr and build actions.

@RaphiMC
Copy link
Contributor Author

RaphiMC commented Oct 20, 2023

I have no idea on how to do that

@onebeastchris
Copy link
Member

See here for reference, basically that
It'll allow people to download artifacts

@Kas-tle
Copy link
Member

Kas-tle commented Oct 20, 2023

In pullrequest.yml as well :)

@Kas-tle Kas-tle requested a review from Konicai October 20, 2023 20:34
@Konicai
Copy link
Member

Konicai commented Oct 20, 2023

I have some more small-ish reviews (tonight or tomorrow morning) and I'm hesitant of the current implementation anyway. Only because of what it has been forced to do under the current constraints.

I would really prefer making Geyser's bootstrap system more dynamic, which I would be happy to do quite soon (as well as putting some work into this PR myself)

@onebeastchris
Copy link
Member

On a different note, it would be nice to have some setup info on this on the Geyser wiki - while i assume the setup is similar to the plugin version, we have setup guides for all bootstraps on our wiki (https://wiki.geysermc.org/geyser/setup/). Additionally, a download link would need to be added on the downloads website; iirc @rtm516 could help with that. Regarding the wiki, feel free to reach out in the discord, we can help with that too.

Copy link
Member

@onebeastchris onebeastchris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, just some minor details.
I'm not really a big fan of setting the Floodgate auth type manually, but I'm not sure whether that can be reasonably improved

@Kas-tle
Copy link
Member

Kas-tle commented Feb 15, 2024

Looks good overall, just some minor details. I'm not really a big fan of setting the Floodgate auth type manually, but I'm not sure whether that can be reasonably improved

I don't really see why that's even an issue. There are plenty of other things we set for the user if certain configs are ticked because the other options would do nothing for such a setup.

Copy link
Member

@onebeastchris onebeastchris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides this one thing; LGTM

@onebeastchris onebeastchris dismissed Konicai’s stale review February 16, 2024 11:37

outdated review, requested changes were implemented

@onebeastchris onebeastchris merged commit aca368e into GeyserMC:master Feb 19, 2024
1 check passed
@RaphiMC RaphiMC deleted the feature/viaproxy-platform branch May 7, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Feature When a PR implements a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants