ocurrent / ocurrent-deployer

A pipeline that deploys unikernels and other services
MIT License
22 stars 19 forks source link

Refactor to remove abstraction leak into `cluster.ml` #233

Closed shonfeder closed 1 month ago

shonfeder commented 2 months ago

The current code declares a type like

type service =
  [ `Ci of string
  ; ...
  ; `Ocamlorg_v3c of string 
  ]

Values of this type are used to mark a field in the deployed services in pipeline.ml. Values of this type are not used anywhere in the project except for cluster.ml. In that location, they are used to map to different applications of pull_and_serve. Once we remove the unused AWS entry, every application of pull_and_serve is uniform except that different modules are packed into arguments. The only difference between these modules is a string value provided when applying the Current_docker.Make to contruct instances of Current_docker.S.DOCKER. And the string value provided there exposes information that we should only know if you are within the context of a particular deployer.

This PR tries to simplify the situation via the following change: instead of pre-applying Current_docker.Make to special strings, and then using a map from the service type to select the Current_docker.S.DOCKER we need, we move the special string value to the service configuration in pipeline.ml, and use it directly to construct the needed Current_docker.S.DOCKER. This simplification results in the following benefits:

A few other incidental cleanups are made along the way. Review by commit may be useful, tho the diff is not as big as it looks: more than half the new lines are just from the updated services.md, which has a bit more data now and improved organization.

shonfeder commented 2 months ago

I would like to sync with @mtelvers to discuss the handling of the value used for the docker context, as I think I am abusing that value in the changes to the services.md a bit, so I'll leave this as a draft until I get a chance to do so.

shonfeder commented 1 month ago

I've updated this to resolver merge conflicts, and it should be ready for review again.

shonfeder commented 1 month ago

Thanks for the review and improvements, @mtelvers! I added one fix following your correction about the docker context for the base image builder, and one further refactor to make the construction of the docker modules more obvious.

Should be ready for another look when you have time!

shonfeder commented 1 month ago

Oh, I misunderstood! Thanks for the fix. I agree the naming of the function makes sense.

shonfeder commented 1 month ago

Thanks again for the review and collaboration here! I'm happy with the result :) :raised_hands: