Better error messages for generated manifests #2599
Conversation
squaremo
force-pushed
the
better-fluxyaml-sources
branch
from
08a4ea2
to
2a27ead
Compare
|
It's looking much better now, thanks! Could you add a test? |
Yus. And in fact, in doing so I discovered that duplicates among generated manifests are not detected, because cluster/kubernetes/resource/load.go doesn't attempt to detect them. What do you reckon, should I --
|
Can you elaborate? That file does detect duplicates at |
In |
squaremo
force-pushed
the
better-fluxyaml-sources
branch
from
809108a
to
1e2abd6
Compare
Ah, now I understand, thanks. I was assuming that iterating over the resources, looking for duplicates after parsing was enough. But it isn't, since Thanks! This ended up being more work that one would expect. |
squaremo
force-pushed
the
better-fluxyaml-sources
branch
from
1e2abd6
to
8034389
Compare
| id := item.ResourceID().String() | ||
| if _, ok := objs[id]; ok { | ||
| return nil, fmt.Errorf(`duplicate definition of '%s' (in %s)`, id, source) | ||
| } |
There was a problem hiding this comment.
Can we somehow factor out the duplicate-check and message? maybe in a local function?
There was a problem hiding this comment.
It occurred to me too, but I didn't bother because it would increase the line count, and the sites are close together. Have I drunk too much Go-aid? Possibly.
- The relative path to the `.flux.yaml` from a particular ConfigFile won't change over its lifetime, so we can calculate it when creating the ConfigFile. Then there's no need to check for errors later, which is more convenient. - record the "original" path, that is the path as supplied by the user (which may be `.`, implicitly). This can then be used to further identify where particular manifests came from, especially in errors.
When collecting all the generated resources in configaware.go, we have an idea of where each one came from because it either has a config file attached to it or not. So we can report quite accurately where duplicate definitions arise -- they will either duplicate a resource in a YAML file (no config file attached), the config file currently being processed, or a config file previously processed. To show the config file path as well as the --git-path in question, I've combined them into a gitpath-plus-relative-path. For example, if the original path is `staging`, but the .flux.yaml found is actually in the directory above, it'll use `staging/../.flux.yaml` as the location of the config file.
This gives the various paths clearer names, by appending `Relative` to those which are supplied or calculated as relative to another. The method `ConfigRelativeToWorkingDir()` gives the path to the config file, starting from the working directory. This encodes two pieces of information: the working directory aka `--git-path`, and where the config file is in the git repo. This is also used as the Source of generated manifests (since it will always end in the configuration file name, it'll still work in comparisons e.g., in pkg/daemon/sync.go).
When asssembling manifests from files or generators, there are some checks for duplicate definitions. However, duplicates _within_ a (multi-doc) file, or within a generated set of manifests, won't be detected, since the code that parses a chunk of bytes (in cluster/kubernetes/resource/load.go) doesn't check for duplicates. So: add a test that duplicates within a generated set of manifests are detected, and make it pass by checking when parsing bytes into manifests.
To wit: - when a manifest appears in a YAML file, and is also generated - when a manifest is generated by two different configs
squaremo
force-pushed
the
better-fluxyaml-sources
branch
from
8034389
to
d04cb0d
Compare
This PR improves the error messages for generated manifests, mainly by including the root path -- i.e., the --git-path value, or implicitly
.-- as well as the path to the config file.flux.yaml, given relative to that.For example, in the case of duplicate resources, here's the error message logged:
NB this error message also names the resource in question, therefore fixes #2595.