Fix missing root dir prefix when loading dynamic config

[git-hulk]

The cadence server would fail to load the dynamic config file
when the working dir wasn't the parent dir of dynamic config
file path.

What changed?

Fix missing root dir prefix when loading dynamic config

Why?

The cadence server would fail to load the dynamic config file
when the working dir wasn't the parent dir of dynamic config
file path.

How did you test it?

local test

Potential risks

no

[git-hulk]

@longquanzheng Can you have a look at this PR?

[longquanzheng]

@longquanzheng Can you have a look at this PR?

Thanks for finding and fixing it.

[longquanzheng]

Actually, this will break the case that using absolute path.
Maybe we should check if a path is absolute before transformation

[git-hulk]

Actually, this will break the case that using absolute path.
Maybe we should check if a path is absolute before transformation

I didn't check the path since the comment says that should be relative to the root directory

// Filepath should be relative to the root directory
DynamicConfigClient dynamicconfig.FileBasedClientConfig `yaml:"dynamicConfigClient"`

[longquanzheng]

Actually, this will break the case that using absolute path.
Maybe we should check if a path is absolute before transformation

I didn't check the path since the comment says that should be relative to the root directory

// Filepath should be relative to the root directory
DynamicConfigClient dynamicconfig.FileBasedClientConfig `yaml:"dynamicConfigClient"`

I see. That is misleading, sorry for that. We should remove the comment.

maybe that’s the initial intent but the code doesn’t restrict it. As a result, people are using it with absolute paths now. See https://uber-cadence.slack.com/archives/CNC31V3K7/p1616084908003400?thread_ts=1616084757.003300&channel=CNC31V3K7&message_ts=1616084908.003400

so I think we have to support it anyway. Otherwise I don’t think those people have a workaround

[git-hulk]

so I think we have to support it anyway. Otherwise I don’t think those people have a workaround

Okay, I'll fix it and remove the comment later 👍

[longquanzheng]

so I think we have to support it anyway. Otherwise I don’t think those people have a workaround

Okay, I'll fix it and remove the comment later 👍

Cool. Can you do the same for loading config? I think it’s okay if config is outside root directory. This will probably make it easier/simpler to use homebrew. And also add description to the root CLI parameter: it will not apply to absolute path.

[git-hulk]

so I think we have to support it anyway. Otherwise I don’t think those people have a workaround

Okay, I'll fix it and remove the comment later 👍

Cool. Can you do the same for loading config? I think it’s okay if config is outside root directory. This will probably make it easier/simpler to use homebrew. And also add description to the root CLI parameter: it will not apply to absolute path.

Done, you can have a look again

[longquanzheng]

Just a comment about the CLI parameter. The rest LGTM