fix(core): correctly apply source.path in VCS logic #5305
Conversation
| @@ -0,0 +1 @@ | |||
| We're putting the new cover letter on all of these now, Peter. Didn't you get the memo? | |||
core/src/vcs/vcs.ts
Outdated
| @@ -462,7 +462,13 @@ export function getConfigFilePath(config: ModuleConfig | BaseActionConfig) { | |||
| } | |||
|
|
|||
| export function getConfigBasePath(config: ModuleConfig | BaseActionConfig) { | |||
There was a problem hiding this comment.
Might be good to add a comment to this function explaining that it either returns the actual config file path or user defined path if source.path is set.
Also wondering if there's a better name for this function. Like resolvedActionPath or similar.
WDYT?
| @@ -462,7 +462,13 @@ export function getConfigFilePath(config: ModuleConfig | BaseActionConfig) { | |||
| } | |||
|
|
|||
| export function getConfigBasePath(config: ModuleConfig | BaseActionConfig) { | |||
| return isActionConfig(config) ? config.internal.basePath : config.path | |||
| if (isActionConfig(config)) { | |||
| const basePath = config.internal.basePath | |||
There was a problem hiding this comment.
In a similar vein, would be good to clarify what these different paths actually refer to.
config.internal.basePath and config.source.path don't really tell me anything.
If it's a big refactor to rename these, maybe we can just start by clarifying with code comments.
And tbh, I'm not sure what could be better.
Something like configFilePath and resolvedActionPath?
There was a problem hiding this comment.
I added some docstrings to the config.internal interface and renamed the action.basePath() method to action.sourcePath(), hope that clarifies things a bit.
I didn't see a straightforward way to reduce the number of these fields, since e.g. config.source can have template strings and the remote source path stuff hasn't been taken care of when the configs are scanned from disk.
Before this fix, we weren't accounting for the `source.path` config option for actions when determining the path passed to `getFiles`. This meant that actions using `source.path` would essentially have their versions computed using an empty file list.
thsig
force-pushed
the
source-path-fix
branch
from
f389011
to
126a057
Compare
|
@eysi09 Do you have any further comments on this one, or should we get it merged? |
|
I'm testing this manually now, against the example where I bumped into this. |
This reverts commit aaaf6d5, which causes a regression in certain projects using config templates. I'll open another PR to reintroduce the changes that were reverted here, with the relevant fixes and added test cases.
What this PR does / why we need it:
Before this fix, we weren't accounting for the
source.pathconfig option for actions when determining the path passed togetFiles.This meant that actions using
source.pathwould essentially have their versions computed using an empty file list.Special notes for your reviewer:
As you can see, this is a pretty conservative fix that makes the minimal required changes to resolve the problem.
The naming of
config.internal.basePathandaction.getBasePath()is confusing. The relevant method on the VCS handler receives an action config (not an action), so thegetBasePathmethod isn't available.The helper I changed is very similar to the
getBasePathmethod, but it doesn't factor inremoteSourcePath(which is available to actions, but not action configs).I haven't given it any thought yet, but this might be a good opportunity to rename these fields to more clearly distinguish the intended semantics (and avoid more mistakes like this in the future).
Should I take a closer look and see about cleaning up & refactoring some of this, or should we make do with the actual fix for now?