Source dir of extraManifests

[jonsch318]

Currently the extraManifests are sourced from the current working directory where the cli was launched

https://github.com/budimanjojo/talhelper/blob/7380a168284f7c6985f528feb8e513d1823a6b86/pkg/generate/config.go#L185C1-L198C2

Although this can be logical I would prefer if it were sourced relative to the talconfig.yaml.

What do you think?

[budimanjojo]

Good catch! Yeah this should get fixed. Do you want to do the PR or should I do it myself?

[budimanjojo]

After some thinking, I don't know how to do this without breaking the behavior of: #619

[jonsch318]

I can / are in the middle of fixing it. I would propose two solutions to how it works currently:

1. Allow - extra1.yaml to be loaded from the place of cmd invocation like previously and - @extra1.yaml to be loaded relative to the config dir. Since all other path substitutions are with @ already, this wouldn't be two much different maybe. We then add the absolute path and (only for extraManifests) remove the "@" so that the path becomes valid.

2. Always - extra1.yaml but at first try to load from the cmd invocation if it fails try to load from the config dir

What would do you think?

[budimanjojo]

Ah yeah I missed that extraManifests currently is a list of paths without @ prefix.

@mircea-pavel-anton What do you think?

My thought is it will be confusing but personally I always run talhelper command from where the talconfig.yaml is at so I don't really know from a user perspective.

[mircea-pavel-anton]

@budimanjojo

The more I think about it, to be honest, the more I realize that #619 might have been a mistake... To be fully honest, I don't even remember exactly what caused us to discuss and eventually implement it. Maybe it was tab completion for node names? Regardless.

The thing is - I have said this before and I still stand by it - what talherlper aims (should aim?) to do/be is (more or less) kustomzize for Talos Linux. In that regard, to keep things more straightforward (both for users but also for us, working on it), we should probably "steal" some ideas in terms of how the tool behaves.

What I mean by that is that when you do define patches or resources or components in a kustomization.yaml, those paths, if they are relative, are all evaluated in relation to where the kustomization.yaml file is. Sure, you can call kustomize build /path/to/dir but that does not change where the paths are evaluated from.

In that regard, I think that the approach proposed, i.e. using @ to indicate paths relative to the config dir and everything else relative to the place of CLI invocation is not only confusing for end users (it definitely is for me) but also quite a nightmare to implement and maintain over time.

I think that, even with the risk of it being a breaking change, we should set some clear boundaries as to how this behavior is expected to work. My personal thoughts on the matter are that all relative paths should be evaluated in relation to the config file itself, since that's also the behavior we've grown to expect from the likes of kustomize (or even helmfile, I believe).

I would say that the functionality to have paths relative to the place of CLI invocation should be removed entirely and then everything should be evaluated in relation to the config file location.

[budimanjojo]

Looking at the code from #619, I think the current implementation is already correct? Maybe it's just the wrong wording of the PR?

I see the code logic is doing:

1. Get the absolute "base" path of --config-file value.
2. If the value of machineFiles, patches, extraManifests, inlineManifests is a list of string or string, then replace the prefixed @ with absolute "base" path of --config-file plus the value.

This should mean that the value is substituted with the the path relative to the config file?

[jonsch318]

Nope it is definitely not correct currently. For this two examples:

clusterName: my-cluster
endpoint: https://192.168.200.10:6443
nodes:
  - hostname: master
    controlPlane: true
    ipAddress: 192.168.200.11
    installDisk: /dev/sda
    extraManifests:
      - extra1.yaml

Now he checks if it is in the cmd invocation dir. As expected from the code since there is no '@'

Otherwise if we have

clusterName: my-cluster
endpoint: https://192.168.200.10:6443
nodes:
  - hostname: master
    controlPlane: true
    ipAddress: 192.168.200.11
    installDisk: /dev/sda
    extraManifests:
      - "@extra1.yaml"

An error occurs for 'extraManifests[0], "stat @${TALCONFIG_DIR}/extra1.yaml: no such file or directory"' so the '@' isnt replaced thats all :).

I find the "@" really confusing here since it is directly loaded by talhelper and not like the other ones by the talosctl cmd.

[jonsch318]

I thought I'd like to rewrite the pathsub logic so that it operates on the parsed (not validated!) yaml content. So on the Talconfig type. Because then we would not have to use this (a bit hacky) way and can use golang types

The problem is that the config type is in the same package as the config loader which uses the pathsub i.e. dependency cycle

So i would like to rewrite the path sub and move the config type to its own basic package. For the user this shouldn't affect anything

[budimanjojo]

Looks like you're correct. extraManifests is broken. It's not working with or without "@" prefixed when called outside of configdir.

@jonsch318 you want to work on this?
The easiest way is to just modify the getFileContent() function, but you can try your own way too. I can do it myself too if you're not willing to. I think I need to find time to clean up the code a bit 😆