Closed MatteoVoges closed 4 months ago
Will add tests for compose_node_name
so that it won't happen again, that we break it!
Currently this test fails:
def test_compile_not_matching_targets(self):
with (
self.assertLogs(logger="kapitan.targets", level="ERROR") as cm,
contextlib.redirect_stdout(io.StringIO()),
):
# as of now, we cannot capture stdout with contextlib.redirect_stdout
# since we only do logger.error(e) in targets.py before exiting
with self.assertRaises(SystemExit) as ca:
unmatched_filename = "inventory/targets/minikube-es-fake.yml"
correct_filename = "inventory/targets/minikube-es.yml"
os.rename(src=correct_filename, dst=unmatched_filename)
sys.argv = ["kapitan", "compile"]
try:
main()
finally:
# correct the filename again, even if assertion fails
if os.path.exists(unmatched_filename):
os.rename(src=unmatched_filename, dst=correct_filename)
error_message_substr = "is missing the corresponding yml file"
self.assertTrue(" ".join(cm.output).find(error_message_substr) != -1)
And this makes sense, because the test tests actually unintended behavior.
SystemExit
andis missing the file
.And this makes sense, because the test tests actually unintended behavior.
- If the file name and target name does not match, I don't want a
SystemExit
and- I don't want to search the logs for
is missing the file
.
The error happens in the validate_matching_target_name
function. Because I modified the examples to use the name ${_reclass_:name:full}
, which is the filename, the test does not succeed anymore.
Now the question is, if we handle a mismatch of filename
and parameters.kapitan.vars.target
?
And I surely will modify the test.
Now the question is, if we handle a mismatch of
filename
andparameters.kapitan.vars.target
? And I surely will modify the test.
Context, we added this behaviour before starting using ${_reclass_:name:short}
, and it was to catch the case in which someone would copy the target file without changing the target
parameter properly. Nowadays we should discourage to set the target manually, making it automatically get the value from the filename.
Reasoning...
I usually have this setup:
target: ${_reclass_:name:full}
target_name: ${_reclass_:name:short}
target_path: ${_reclass_:name:path}
...
kapitan:
vars:
target: ${target}
IMHO compose_target_name=True
should be interpreted as "automatically infer the target name from the directory/path", so a mismatch between parameters.kapitan.vars.target
and the "composed" target name should fail
for inventory/targets/test1/test2.yml
with compose_target_name=True
:
${_reclass_:name:short}=test2
${_reclass_:name:full}=test1.test2
path/filename
should match test1/test2
(just filename check is not enough because it could be in the directory test3/
)
I think the test should fail if this is not true, and warn that you should not override the target manually
However with compose_target_name=False
:
${_reclass_:name:short}=test2
${_reclass_:name:full}=test2
we should allow for someone to change the target in cases like:
test1/test2.yml
test3/test2.yml
as otherwise they would both be test2
and we want to allow for this manual setup.
I believe long term compose_target_name=False
should be the default, perhaps in the upcoming release?
partially implemented by https://github.com/kapicorp/kapitan/pull/1164 which was an urgent fix. Please rebase from that.
Proposed changes
compose-node-name
compose-node-name
tocompose-target-name
, because it should be kapitan-lingocompose-node-name
deprecatedcompose-target-name
for reclasscompose-target-name
(no breaking changes, just improvements)cached.args