add privileged to host config

[mfleader]

Changes introduced with this PR

Add privileged option to deployment host config.

---

By contributing to this repository, I agree to the contribution guidelines.

[webbnh]

Doesn't this PR need the same additions that you did for arcalot/arcaflow-engine-deployer-podman#58?

[dbutenhof]

Doesn't this PR need the same additions that you did for [...]

I had the same thought, but earlier I didn't try to look. It does seem, however, that the other schema properties are individually referenced and there's no generic mechanism to process them all; so, yeah, I think there should be. (Though it looks very different for docker as it uses a REST API client rather than forking a cli command.)

[mfleader]

Doesn't this PR need the same additions that you did for [...]

I had the same thought, but earlier I didn't try to look. It does seem, however, that the other schema properties are individually referenced and there's no generic mechanism to process them all; so, yeah, I think there should be. (Though it looks very different for docker as it uses a REST API client rather than forking a cli command.)

Correct, the docker deployer does not use the cli. Instead, it communicates with the docker api through a golang client api. Our docker deployer's config gets unmarshalled into a container.HostConfig struct as the Deployment section of the docker deployer config. The Deployment.HostConfig is passed to the docker golang client, and used to create containers.

[webbnh]

Our docker deployer's config gets unmarshalled into a container.HostConfig struct as the Deployment section of the docker deployer config.

That's very cool!...but shouldn't this PR include a test which proves that it works? (I.e., which shows that the Privileged field is now accepted?)

[dbutenhof]

OK, so the schema just gets automagically converted into a Docker HostConfig struct and passed through. Cool, if a bit mysterious.

I agree it would be nice to have tests ... but there's very little testing infrastructure in place and it amounts to a minimal functional test that a container can be deployed and generates output. 😦 I don't like the additional accumulated debt here, but one might also argue that placing the entire burden of building a deployment unit test environment from scratch is a bit much to put on this little PR.

[mfleader]

Our docker deployer's config gets unmarshalled into a container.HostConfig struct as the Deployment section of the docker deployer config.

That's very cool!...but shouldn't this PR include a test which proves that it works? (I.e., which shows that the Privileged field is now accepted?)

No additional testing is needed because the schema uses StructMappedObjectSchemas for the ContainerConfig and the HostConfig which maps our golang schema structures into the exact config structures used by the container api, container.Config and container.HostConfig, respectively. If the key had been typed as Privilege there would be an error saying the value cannot be set because the Privilege key could not be found because it does not exist in container.HostConfig.

[jaredoconnell]

Looks good. It's good that this field was already present on the structure.

[webbnh]

If the key had been typed as Privilege there would be an error saying the value cannot be set because the Privilege key could not be found because it does not exist in container.HostConfig.

I believe you, but do we have a test which supports that assertion? (Are there tests for NewStructMappedObjectSchema(), in particular with the container.HostConfig specialization? I don't see any tests for the schema.go module.)