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

Remove Server's dependency on Controller classes #479

Merged
merged 11 commits into from
Nov 25, 2024

Conversation

ThanGerlek
Copy link
Contributor

@ThanGerlek ThanGerlek commented Nov 14, 2024

Introduces an EndpointProvider interface which Server now uses for all its endpoint calls. Also extracts startup logic (getOptions, initializeDaos, etc) out of the Server class and into a Main class.

Warning

This moves the location of the main() method (from java/server/Server.java to java/Main.java). Existing run configurations will likely break and will need to be updated.

@ThanGerlek
Copy link
Contributor Author

@19mdavenport Since this moves the main() method into a different file, how will this affect the existing deployment system? Does anything else need to happen?

The good news is that it's now in a dedicated Main class, so it will probably never need to move again.

@ThanGerlek ThanGerlek force-pushed the reduce-endpoint-dependencies branch from 00344a5 to 3a7241d Compare November 16, 2024 23:30
@ThanGerlek ThanGerlek marked this pull request as ready for review November 19, 2024 00:46
@ThanGerlek ThanGerlek marked this pull request as draft November 19, 2024 00:47
@ThanGerlek ThanGerlek marked this pull request as ready for review November 19, 2024 01:07
@ThanGerlek ThanGerlek requested a review from webecke November 19, 2024 01:35
Copy link
Contributor

@webecke webecke left a comment

Choose a reason for hiding this comment

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

LGTM!

Seems like a clean solution to make things more independent of each other and more flexible. It does look like adding new endpoints now requires a little more work, but its marginal so I'm not too worried about it. Great work!


private static EndpointProvider endpointProvider = new EndpointProviderImpl();

public static void main(String[] args) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you fix the main class configuration in pom.xml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated mainClass, is that all that needs to happen?

Sorry, I know absolutely nothing about Maven 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think so, you could try testing this by running mvn exec:java in the base directory and seeing if the server starts normally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, did you commit/push that update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but in the wrong branch. RIP.

@frozenfrank
Copy link
Contributor

frozenfrank commented Nov 21, 2024 via email

@frozenfrank
Copy link
Contributor

frozenfrank commented Nov 21, 2024 via email

@ThanGerlek
Copy link
Contributor Author

ThanGerlek commented Nov 21, 2024

@19mdavenport Apparently mvn exec:java doesn't work for me (I get this error message (although the listed solutions don't fix it for me), even on the main branch in a dev container), so someone other than me might need to test that. Obviously it does pass our CI/CD tests but I assume they have a different execution path

@ThanGerlek ThanGerlek force-pushed the reduce-endpoint-dependencies branch from 2759b38 to 33829fa Compare November 25, 2024 18:06
@ThanGerlek ThanGerlek merged commit 16eec28 into main Nov 25, 2024
2 checks passed
@ThanGerlek ThanGerlek deleted the reduce-endpoint-dependencies branch November 25, 2024 18:12
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.

4 participants