search

unioslo / zabbix-auto-config

MIT License
3 stars 6 forks source link

Add utils.py Tests #46

Closed pederhan closed 1 year ago

pederhan commented 2 years ago

Changes

This pull request adds test coverage for the functions in utils.py.

This is a draft, and I am looking for feedback on the intended behavior of certain functions in the utility module. As I work on this, I will ask for clarification in subsequent comments.

pederhan commented 2 years ago

read_map_file

For this function, I am wondering whether the fact that this: https://github.com/pederhan/zabbix-auto-config/blob/08694a55a813fda216b69209baeee8603acb77ed/tests/test_utils.py#L18-L22

evaluates to this: https://github.com/pederhan/zabbix-auto-config/blob/08694a55a813fda216b69209baeee8603acb77ed/tests/test_utils.py#L37-L40

is intended behavior or not?

It seems to me like key4 and key5 evaluating to [""] could be fine, while key6 evaluating to ["", ""] and key7 evaluating to ["val7", ""] is potentially undesirable.

paalbra commented 2 years ago

is intended behavior or not?

Short history and comment:

The map file format predates this project. Little effort was made to make read_map_file very robust or strict. It's kindof implemented as a minimal viable product. I.e. it'll work, as long as you know what you're doing with the config.

Your remarks are very valid. Some of the valid lines (i.e. gets parsed without error) will probably result in buggy behavior.

I think it's a good idea to make it more robust and strict, if you'd like to do that @pederhan ?

Some possible problems and problematic behavior:

Will probably result in bugs/crashes(?):

Empty keys:

:1,2,3

Empty values:

a:1,,3

Will probably not lead to bugs, but it's confusing behavior and could lead to human error:

Duplicate values:

b:1,1,2,3

Duplicate keys:

c:1,2,3
c:4

Personally I think it would be nice to add prevention mechanisms for all of the above.

pederhan commented 2 years ago

Got it, thanks for the insight. I'll try to get on it as soon as possible.

pederhan commented 2 years ago

I have one question regarding the use of colons in values.

In the test added in 58de4f90342c767f24adb46e65714c51c4641abf, we have a case where a line is i:7:8, and it is parsed as key="i", value="7:8" to allow for colons in values.

If colons should strictly not be allowed in values, then let me know and I'll remove it.

paalbra commented 2 years ago

I have one question regarding the use of colons in values.

In the test added in 58de4f9, we have a case where a line is i:7:8, and it is parsed as key="i", value="7:8" to allow for colons in values.

If colons should strictly not be allowed in values, then let me know and I'll remove it.

IMO there is no reason to be very strict "just because". Like PEP20: "Simple is better than complex". I think as a internal policy one should not create weird values, like 7:8, but if someone using the project really wants to: Why not allow it? 🤷‍♂️