Convert "TODO" strings to change in the template by external variables

emmanuelbernard commented 2 years ago

Description

A decent portion of the todo in the code are static strings, service name, API path etc. A way to make a template user life easier and reduce the number of non rebasable code would be to externalize these strings and read them from an external config file, env variable and or config map. That would make life easier for consumers and might not make the template too terribly more complicated.

Analysis

See description, a decent % of TODO grunt would could be simplified and centralized into an external config file. How doable it is in Go is to be analyzed as I am less than an novice in it.

Task List

emmanuelbernard commented 2 years ago

Cc @pb82 @miguelsorianod

emmanuelbernard commented 2 years ago

also ccing @machi1990

machi1990 commented 2 years ago

I can see benefits in doing it for some things e.g API path, routes etc but I'll be cautious in doing it for all the TODO comments for a static string as some of those are not really meant to be configurable. I am saying that because we already have a lot of CLI flags, adding more for things that are only meant to be changed once is likely going to make it harder for the user on that aspect.

miguelsorianod commented 2 years ago

I agree with manyanda's comment

emmanuelbernard commented 2 years ago

@machi1990 walk me through it. Why would these be CLI flags? Why not be a mandatory config file deployed alongside the binary. It will be a build time artifact. If I rephrase my proposal, let's externalize build time strings that vary from one fleet manager to another into a configuration file that is a build time artifact used and integrated in the fleet manager image.

miguelsorianod commented 2 years ago

Maybe we could explore leveraging the use of Go embed (available since Go 1.16 https://blog.jetbrains.com/go/2021/06/09/how-to-use-go-embed-in-go-1-16/) or go-bindata (https://github.com/go-bindata/go-bindata) for that. Definitely something potentially worth exploring for some of those cases

machi1990 commented 2 years ago

@machi1990 walk me through it. Why would these be CLI flags?

Because that's the way configuration are exposed at the moment in the template.

Why not be a mandatory config file deployed alongside the binary. It will be a build time artifact.

kas-fleet-manager did not have the classification of build time / runtime configuration as most configuration were more runtime in nature.

If I rephrase my proposal, let's externalize build time strings that vary from one fleet manager to another into a configuration file that is a build time artifact used and integrated in the fleet manager image.

Thanks for the clarification. @miguelsorianod comment https://github.com/bf2fc6cc711aee1a0c2a/ffm-project/issues/8#issuecomment-1041309017 above could be something to explore. That might reduce the number of "TODO" in relation to static strings to possibly "just one" in this file.

machi1990 commented 2 years ago

I'd look at this to see what can be done.

machi1990 commented 2 years ago

Maybe we could explore leveraging the use of Go embed (available since Go 1.16 https://blog.jetbrains.com/go/2021/06/09/how-to-use-go-embed-in-go-1-16/) or go-bindata (https://github.com/go-bindata/go-bindata) for that. Definitely something potentially worth exploring for some of those cases

I've looking at this and what it means is:

Define a configuration file where those constants will live
Define a way to load the file during compilation time. The output of the loading will either be a string, bytes or special embedded data structure, this output will then need to be parsed into a struct (a container of these constants) that we can use in the code.
Finally, pass the final struct throughout the codebase and use the needed constants.

The advantage is:

The constants will be modified only in this configuration file

The disadvantage are:

added complexity (see the workflow above) for constants that are only made to modified once (after the template has been cloned)
Since the loaded file content will be parsed during runtime, the constants are not compilation time only, their value will be seen/evaluated during runtime
Now we'll have two ways of defining configuration (via flags and this embed mechanism)

I am not convinced that this is the way we should go. But still, having a TODO comment on top of the hardcoded and not-commented constants is something we should look into addressing to make the template more approachable. For that, I was thinking of moving some of the constants ("the only ones that we expect that the use will need to change") in the constants directory, which just contains a bunch of golang files. Make sure that the usage of these values are documented/commented.

In the end, we could have something like: https://github.com/bf2fc6cc711aee1a0c2a/ffm-fleet-manager-go-template/blob/9026405f2185ec2f7fc22d8853114367eb67f9ef/internal/dinosaur/constants/dinosaur.go#L13-L48

@emmanuelbernard Let me know what you think about that proposal.

/cc @miguelsorianod

machi1990 commented 2 years ago

I've opened https://github.com/bf2fc6cc711aee1a0c2a/ffm-fleet-manager-go-template/pull/23 to expose the API base path as a config.

emmanuelbernard commented 2 years ago

I don't intuitively think go embed or go bin data is the way to go. @pmuir mgiht have some alternative ideas.

The proposal I had was, assuming one could read properties from a file at runtime, to have:

a config file that holds all these "build time" or rather non SRE time properties
have that config file be embedded into the fleet manager container image
have this file be read at startup time of the fleet manager (from the container image) to parameterize the TODO properties.

miguelsorianod commented 2 years ago

I don't intuitively think go embed or go bin data is the way to go. @pmuir mgiht have some alternative ideas.

The proposal I had was, assuming one could read properties from a file at runtime, to have:
* a config file that holds all these "build time" or rather non SRE time properties

* have that config file be embedded into the fleet manager container image

* have this file be read at startup time of the fleet manager (from the container image) to parameterize the TODO properties.

What is the advantage of this approach over the embed or go-bindata approach? If I understood correctly your comment, with both approaches the data is configured at build time. The difference is that the build time artifact where you attach that configuration file to. In the embed approach it is included in the binary, In the container approach it is included in the container image. Additionally, with the container approach as you said the file would be read at runtime, whereas with go embed that wouldn't necessarily be the case, so on the performance side the embed approach would be more desirable.

Aside from that, is it really worth doing? The number of TODO constants does not seem very high, and an easier approach would just be centralize them in a .go file. What is the big advantage that we would get?

emmanuelbernard commented 2 years ago

What is the advantage of this approach over the embed or go-bindata approach? If I understood correctly your comment, with both approaches the data is configured at build time. The difference is that the build time artifact where you attach that configuration file to. In the embed approach it is included in the binary, In the container approach it is included in the container image. Additionally, with the container approach as you said the file would be read at runtime, whereas with go embed that wouldn't necessarily be the case, so on the performance side the embed approach would be more desirable.

Aside from that, is it really worth doing? The number of TODO constants does not seem very high, and an easier approach would just be centralize them in a .go file. What is the big advantage that we would get?

Your description is correct.

One of the drawback of the embed / go-bindata approach is the more convoluted workflow @machi1990 described in his analysis, that is what I was mainly reacting to. We can go for a centralized Go struct which would work great for a one off template. And it might not be too extra complicated to reapply the new version of the template due to the centralization. Would this analysis be different in a future library model? It would not if you were to pass a Config struct of sort in and use duck typing to accept that Config object defined in the fleet manager to be accepted by the Config structure defined in the library.

bf2fc6cc711aee1a0c2a / ffm-project