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

Move checker-related stuff to its own repository #161

Open
ldruschk opened this issue Jun 26, 2021 · 5 comments
Open

Move checker-related stuff to its own repository #161

ldruschk opened this issue Jun 26, 2021 · 5 comments

Comments

@ldruschk
Copy link
Member

In my opinion, there is no reason why the C# EnoChecker is placed in the EnoEngine repository. Moving this to its own repository would help clarifying which part of the code are necessary to interface with any checker of the specified interface and which parts are relevant only to the checker server implementation in C#.

If there is any re-used code between the engine and C# checker, that should be cleanly placed in its own package.

@DanielHabenicht
Copy link
Contributor

yep would be cool to have it in a enochecker-dotnet repository and renaming the other checkers accordingly, e.g. enochecker (which is python) to enochecker-python

@Trolldemorted
Copy link
Member

I agree that a refactoring is necessary. Our requirements are:

  • Code must be shared between EnoChecker, EnoEngine, EnoLauncher and EnoFlagSink
  • Class definitions must be shared between EnoLandingPage Frontend, EnoEngine and EnoLauncher (scoreboard, attackinfo)
  • Class definitions (IChecker and friends) must be shared between EnoChecker runtime and EnoChecker implementations
  • The class definitions package must be as small as possible

Therefore we could split our packages in the following way:

  • EnoCore.Models contains all basic class definitions that are shared between projects. It does not have any dependencies.
  • EnoCore depends on EnoCore.Models.
  • EnoDatabase remains like it is and depends on EnoCore.
  • All logging-related functions and methods remain in EnoCore. Engine, Launcher, FlagSink and Checker depend on it (either directly or through EnoDatabase).
  • EnoChecker is moved out of this repository.
  • IChecker and friends follow EnoChecker and form a new project, e.g. EnoChecker.Core.

@DanielHabenicht
Copy link
Contributor

DanielHabenicht commented Jun 26, 2021

IChecker and friends follow EnoChecker and form a new project, e.g. EnoChecker.Core.

Are you sure you want another package for Enochecker? I would just publish EnoChecker as it is in the new Repository. I think we both mean the same: Publishing EnoChecker as a new package.

@DanielHabenicht
Copy link
Contributor

DanielHabenicht commented Jun 27, 2021

might it be useful to have PRs for refactoring this stuff?
I think you have a typo in here:
9c7139f#diff-9bc0aa9f51ff6aedff652c211fa7771aff85093ba5c42432e263d0565c8b9e34R1

And in my last PR we talked about that everything should be LF, but now we are checking for CRLF?
9c7139f#diff-0947e2727d6bad8cd0ac4122f5314bb5b04e337393075bc4b5ef143b17fcbd5bR8

Whats about the dummy checker should we retain it somewhere for performance tests?

@Trolldemorted
Copy link
Member

I think you have a typo in here:
9c7139f#diff-9bc0aa9f51ff6aedff652c211fa7771aff85093ba5c42432e263d0565c8b9e34R1

oh thanks, I guess we'll tend to that after enowars 5 is over though

And in my last PR we talked about that everything should be LF, but now we are checking for CRLF?
9c7139f#diff-0947e2727d6bad8cd0ac4122f5314bb5b04e337393075bc4b5ef143b17fcbd5bR8

No, you just commited a handful of CR files into an otherwise completely CRLF repo

Whats about the dummy checker should we retain it somewhere for performance tests?

yeah we should keep it with enochecker and provide prebuilt docker images, but that shouldn't be much work

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

No branches or pull requests

3 participants