Docker compose override

Depetrol commented 4 months ago

Added docker-config-file option to docker target options. This allows the user to provide a custom docker-compose file that will add and override the generated docker-compose file using this feature provided by docker.

The design of the docker-config-file is similar to env-file. The user specifies a docker-config-file parameter in the docker target as the path to a custom docker compose yaml file.

docker: {
    docker-config-file: "./DockerComposeConfig.docker.yml"
}

The user-specified yaml file will be copied as docker-compose-override.yml under src-gen folder. If docker-compose-override.yml exists under the src-gen folder, the script under bin will use docker-compose.yml and docker-compose-override.yml to start the docker containers: docker compose -f docker-compose.yml -f docker-compose-override.yml up --abort-on-container-failure . The specified yaml file should be the same structure as a normal docker-compose.yml file that has each top level federate as a service. To accommodate this syntax, the federate__ prefix has been removed from the generated .lf files for each federate in src folder.

Depetrol commented 3 months ago

CI is failing because of #2376

elgeeko1 commented 3 months ago

Can you explain more about this statement?

To accommodate this syntax, the federate__ prefix has been removed from the generated .lf files for each federate in src folder.

There are other places where we depend on the federate__ prefix, such as our use of k3s or similar that expect the code generator to prefix the federate names. Can we accomplish this without this change?

Depetrol commented 3 months ago

There were some previous discussion about this --

This seems to be breaking some tests in the C target because some .lft files have a "federate" prefix. Alternative could be when copying the user specified yml file, add the prefix federate to each of the service names. However this would introduce another dependency snakeyaml.

The C tests has been fixed. But if there are other places that depend on the prefix, this change can be accomplished without removing the federate__ prefix.

lhstrh commented 3 months ago

There were some previous discussion about this --

This seems to be breaking some tests in the C target because some .lft files have a "federate" prefix. Alternative could be when copying the user specified yml file, add the prefix federate to each of the service names. However this would introduce another dependency snakeyaml.

The C tests has been fixed. But if there are other places that depend on the prefix, this change can be accomplished without removing the federate__ prefix.

I don't think we should have the prefix. Is there any reason anyone can cite for needing it?

Depetrol commented 3 months ago

I have no problem with removing the prefix, both implementations are fine on my side. @elgeeko1 said he has some places that uses the federate__ prefix.

There are other places where we depend on the federate__ prefix, such as our use of k3s or similar that expect the code generator to prefix the federate names. Can we accomplish this without this change?

elgeeko1 commented 3 months ago

To accommodate this syntax, the federate__ prefix has been removed from the generated .lf files for each federate in src folder.

It's not clear from your comment where this prefix is being removed. Which files are you proposing to change?

For better or worse, the LF code generator uses federate__ as a prefix for the binary names, folder names, and docker compose services names for all files it generates. Consequently there are downstream users who expect this.

Why is this change necessary? Does this mean that the names of the services would be different depending on whether I used an override file or not? If so then it's problematic that my service names would change between the two use cases.

elgeeko1 commented 3 months ago

Shulu and I had a chance to chat, here's my understanding:

A docker compose override file overrides the behavior of the parent docker compose file by overriding properties of a given service. The service is identified by its service name as defined in the parent docker compose file. The proposal currently in this PR is to have the user-provided docker compose override file reference service names by their LF filename and hence absent the federate__ prefix, which may be more user-friendly. LF compiler will then parse the user provided override file, parse the service names, and produce a new override file in fed-gen with the federate__ prefix so that the service names match the docker compose file currently generated by LF.

My concerns with this are:

This introduces a new feature -- it's a small one, but it is a feature, that we use friendlier names in our override file, and have code that parses this file and generates a modified version into fed-gen. This would need to be tested and maintained. It adds a new error pathway, for example, if the YAML parsing fails (which will otherwise happen when the docker compose build is run).
It is confusing to the user that the names of the services in the override file do not match what actually gets created in the containers. The service in my override file myreactor should override the myreactor service in the original docker compose file, and the container name and hostname of the container will default to myreactor. However, in this case, the service name myreactor in the override file actually overrides a service federate__myreactor and its container name and hostname will be federate__myreactor, which is confusing and disagrees with the docker specification.
An alternate solution is to modify the generated docker compose file in LF to omit federate__ but this will likely cause breaking changes elsewhere.

I agree that the service names without federate__ are easier to read and write, and this fits with many other usability improvements that could be made. For now, I think we should stick with the simplest solution that is consistent with the docker standard and the docker compose file currently generated by lfc.

That's my only concern with this PR. Outside of this concern, this is a very helpful feature that I will plan to use!

lhstrh commented 3 months ago

I'm happy as long as we don't introduce an extra parsing step for the same reasons @elgeeko1 already pointed out.

Depetrol commented 3 months ago

I added a command to build RTI docker image in the CI testing environment. Otherwise a docker RTI image version mismatch would cause the test to fail. I think we can merge this then?

lhstrh commented 3 months ago

I added a command to build RTI docker image in the CI testing environment. Otherwise a docker RTI image version mismatch would cause the test to fail. I think we can merge this then?

Wouldn't it suffice to use the option docker: {rti-image: "rti:local"}? See docs here.

Depetrol commented 3 months ago

I think using docker: {rti-image: "rti:local"} in a test is undesirable -- we would have to add docker: {rti-image: "rti:local"} to all docker related test files. Changing the RTI image in the testing environment is more general and allows the test to better simulate user behavior.

lhstrh commented 3 months ago

I think using docker: {rti-image: "rti:local"} in a test is undesirable -- we would have to add docker: {rti-image: "rti:local"} to all docker related test files. Changing the RTI image in the testing environment is more general and allows the test to better simulate user behavior.

We already have...

Depetrol commented 3 months ago

I'll use docker: {rti-image: "rti:local"} then.

Depetrol commented 3 months ago

I created a PR https://github.com/lf-lang/lf-lang.github.io/pull/281 for the updated documentation on docker compose override. (Also could you merge this current PR? I can see that it's been approved but I don't have merge access)

elgeeko1 commented 3 months ago

Great work!

lf-lang / lingua-franca

Docker compose override #2371