insomniacslk commented 3 years ago

contest-generator is a tool used to generate a contest executable with a custom set of plugins. See README.md for instructions.

Signed-off-by: Andrea Barberio insomniac@slackware.it

insomniacslk commented 3 years ago

This is still a work in progress. I have to automatically generate cmds/contest/main.go via github actions, and validate the build.

For now this PR has:

the implementation of contest-generator
a new cmds/contest/main.go obtained by running ./contest-generator
removed cmds/contest/server/ which is now entirely embedded in the main.go. This was probably used for testing, so I'm looking for usage and to replace it with the code generator

insomniacslk commented 3 years ago

I see why the server package. I'll try to fix the e2e tests

codecov[bot] commented 3 years ago

Codecov Report

Merging #297 (dae4b27) into main (884d78e) will decrease coverage by 0.70%. The diff coverage is 31.85%.

:exclamation: Current head dae4b27 differs from pull request most recent head 5f67637. Consider uploading reports for the commit 5f67637 to get more accurate results

@@            Coverage Diff             @@
##             main     #297      +/-   ##
==========================================
- Coverage   69.37%   68.67%   -0.71%     
==========================================
  Files         136      138       +2     
  Lines        7586     7718     +132     
==========================================
+ Hits         5263     5300      +37     
- Misses       1814     1897      +83     
- Partials      509      521      +12

Flag	Coverage Δ
e2e	`47.19% <30.43%> (-0.09%)`	:arrow_down:
integration	`58.95% <ø> (-0.05%)`	:arrow_down:
unittests	`50.27% <32.14%> (-0.72%)`	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmds/contest-generator/main.go	`0.00% <0.00%> (ø)`
cmds/contest/server/server.go	`55.76% <27.27%> (-5.27%)`	:arrow_down:
cmds/contest-generator/config.go	`46.75% <46.75%> (ø)`
plugins/listeners/httplistener/httplistener.go	`42.66% <100.00%> (ø)`
pkg/runner/test_runner.go	`91.91% <0.00%> (-0.60%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 884d78e...5f67637. Read the comment docs.

insomniacslk commented 3 years ago

Note: the listener plugin is not customizable yet, because there is no listener interface. I will add it in a later PR

rojer9-fb commented 3 years ago

i'd suggest a YAML config file instead of a custom txt, for easier future extensibility.

insomniacslk commented 3 years ago

i'd suggest a YMAL config file instead of a custom txt, for easier future extensibility.

The format could be something like:

targetmanagers:
  - github.com/facebookincubator/contest/plugins/targetmanagers/targetlist
  - ...

testfetchers:
  - ...

Is this what you have in mind @rojer9-fb ?

I am not so happy of the mandatory indentation which I expect to lead to confusion, but this is simple enough to minimize the risk

rojer9-fb commented 3 years ago

Is this what you have in mind @rojer9-fb ?

yeah, pretty much

insomniacslk commented 3 years ago

Done. The code is also a lot simpler now, thanks for the suggestion @rojer9-fb !

insomniacslk commented 3 years ago

Addressed feedback from Deomid, and added validation / duplicate checks. Will address Tobias's feedback in the next iteration

insomniacslk commented 3 years ago

I need to address more feedback, and add testing

rojer9-fb commented 3 years ago

LGTM, good to go once Tobias is also happy

insomniacslk commented 3 years ago

split config functions and types into config.go

insomniacslk commented 3 years ago

will add tests for configuration objects after lunch, everything else should be addressed. @tfg13 please take a look, I have left one of your comments unresolved with a question

insomniacslk commented 3 years ago

Tests added. @tfg13 PTAL

insomniacslk commented 3 years ago

fixed tests

mimir-d commented 3 years ago

would you say that adding a build.sh script to do all of what the readme expects the user do to (go build stuff) is helpful? I'm thinking since this PR has the semantics of providing a good to use binary with contest, maybe that goes the extra step for a turn-key solution.

insomniacslk commented 3 years ago

would you say that adding a build.sh script to do all of what the readme expects the user do to (go build stuff) is helpful? I'm thinking since this PR has the semantics of providing a good to use binary with contest, maybe that goes the extra step for a turn-key solution.

Not entirely sure if I got your point, but if you are referring to simplifying the process of building ConTest with contest-generator, there is already a pre-generated main.go under cmds/contest. I think a build script would only add more layers and confusion, and if the user needs to customize the build, they still need to edit a text file and call contest-generator, which is standard process for Go programs.

At the same time, this reminded me that contest-generator needs to be called by GitHub Actions, and rebuild and validate cmds/contest/main.go, which I'm going to do now

mimir-d commented 3 years ago

Yes, I'm talking about simplifying and guiding the user. The thing I have in mind is to have some extra checks for the user (I'm pretty sure a lot of people don't read the manual), like build.sh would point you to $EDITOR for the yaml file, then calls the contest-generator and go build and says "your binary is here".

insomniacslk commented 3 years ago

Yes, I'm talking about simplifying and guiding the user. The thing I have in mind is to have some extra checks for the user (I'm pretty sure a lot of people don't read the manual), like build.sh would point you to $EDITOR for the yaml file, then calls the contest-generator and go build and says "your binary is here".

Honestly I think this is too far. For that kind of users we should just provide prebuilt binaries

insomniacslk commented 3 years ago

since cmds/plugins was removed because redundant, but still used by the migration tools, I moved that code directly into the migration tool

insomniacslk commented 3 years ago

OK this is finally ready for the final review. A recap of the changes:

added contest-generator (cmds/contest-generator/*)
generated cmds/contest/main.go using contest-generator, which includes a comment with the commands to regenerate it
tested contest-generator in GitHub actions so that it generates and validates the new code, compares it to cmds/contest/main.go, and fails if they are different (i.e. if the user forgot to regenerate it upon changes)
removed the now redundant cmds/plugins/ and merged the remaining bits into the only consumer (the migration tool)
included user documentation for contest-generator (cmds/contest-generator/README.md)
provided default configuration file, with example for import aliasing (cmds/contest-generator/core-plugins.yml)
tested parseConfig (cmds/contest-generator/config_test.go)
renamed httplistener.NewHTTPListener to httplistener.New, waiting to create a listener interface in the next PRs (so that listeners will be configurable too - added as TODO)
added TODO to include also user functions into code generation
renamed cmds/contest/server.ServerMain to cmds/contest/server.Main (redundant prefix)
e2e tests now explicitly bubbling up server errors
e2e tests run successfully with non-CI docker-compose setups

insomniacslk commented 3 years ago

tests are green, and also improved error surfacing

facebookarchive / contest

Added contest-generator #297

Codecov Report