Allow specifying whether to make file paths absolute when parsing configs. #5805
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5805 +/- ##
==========================================
- Coverage 71.00% 70.99% -0.02%
==========================================
Files 438 439 +1
Lines 16497 16499 +2
==========================================
- Hits 11714 11713 -1
- Misses 3921 3923 +2
- Partials 862 863 +1
Continue to review full report at Codecov.
|
|
From the description, if setting absolute paths in all configs is only required in |
| } | ||
| // if current config is either a URL or STDIN, base path should be CWD | ||
| if util.IsURL(currentConfigPath) || currentConfigPath == "-" { | ||
| cwd, _ := util.RealWorkDir() |
There was a problem hiding this comment.
maybe add a comment on why we can safely ignore the error here?
| @@ -91,6 +90,7 @@ type SkaffoldOptions struct { | |||
| RPCPort int | |||
| RPCHTTPPort int | |||
| BuildConcurrency int | |||
| MakePathsAbsolute *bool | |||
There was a problem hiding this comment.
Why make it this a *bool? Would opts. MakePathsAbsolute false not suffice?
This is not a command line flag, right?
There was a problem hiding this comment.
Wondering, if we could write something as
yaml.MarshalWithSeparatorWithAbsolutePathsusing theyaml.processTagskind of utility.
We actually need absolute path substitution for all real working modes like dev, debug, build and deploy. So it's not something that can be done only while writing output. The reverse might have worked MarshalWithSeparatorWithOriginalPaths but we don't retain the original paths anywhere, so even that can't be done.
There was a problem hiding this comment.
Why make it this a *bool? Would opts. MakePathsAbsolute false not suffice?
I have 3 modes of parsing (see description). So nillable boolean nil, true, and false works out. Otherwise I can use an explicit struct for parsingMode.
There was a problem hiding this comment.
So, true is set in diagnose workflow. Its nil by default.
When is opts. MakePathsAbsolute set to false? - Do you plan to set it to false in inspect workflow?
There was a problem hiding this comment.
yes, it'll be set to false for skaffold inspect commands. I'm making this change to have a single parser handle all cases and not duplicate config parsing code.
We could do that. But I think having the setting in the |
I would argue we are adding complexity to the code in present thinking about use case in future. |
|
will address requested changes around code complexity in follow up PR. |
Current behavior of the config parser is to preserve the original filepaths specified in the root
skaffold.yamlfile while setting all the imported configs' filepaths to absolute values immediately.#5791 introduced the setting
MakePathsAbsoluteto make the parser substitute absolute file paths for all configs (including the root configs).This PR changes the behavior of
MakePathsAbsoluteto allow specifying not to set absolute file paths for any config.So, now the parser can:
skaffold.yamlneed to be relative toCWDwhere skaffold is invoked, and not the directory of theskaffold.yamlfile.skaffold diagnosethat take a snapshot of the entire set of skaffold configs that can then be invoked from anywhere.skaffold inspectcommands that will recursively modify and write out the set of skaffold configs, it needs to preserve the original relative file paths specified.