template/base: allow hierarchy to include empty directories

[vinny-sabatini]

Description

Allow the hierarchy resolution in the base processParameters.sh script to include directories that do not include any yaml or json files.

In addition, while testing I noticed warnings being logged by the find command due to ordering of options. I have moved the -name flags to the end to address this.

Fixes #184

Type of change

• Bug fix (non-breaking change which fixes an issue)

[akavel]

1. (Note for future: this can be also shortened to [ -n "${YAML_FILES}" ] (as in l. 42), or even [ "${YAML_FILES}" ]).
2. It's not clear to me what's the behavior here with regards to l. 68; do I understand correctly that we'll get this error message now if and only if when there's an empty hierarchy.lst file present in the repo?

[vinny-sabatini]

1. Good call, thanks for the feedback
2. Looking at the conditional on L.65-70, I believe that is checking if /tmp/eunomia_values_processed1.yaml exists, which is created on line 45, so I don't think that error would ever be hit.

The conditional I added will append yaml/json files to /tmp/eunomia_values_processed1.yaml if there are any in the current directory being checked, and will move on if there are not any yaml/json files in the directory.

Hope that helps!

[akavel]

Ad.2: There's a few parts to this story, as I see it:

Firstly, if we are sure this error will never be hit, we should remove it.

However, there's currently one subtle scenario, when I believe it still can be hit there's some tricky behavior: I expect it to be triggered when hierarchy.lst file exists, but is empty. In this case, I expect the code flow would go like this:

1. FOLDERS is set to empty on l.21
2. if on l.23 is entered (because hierarchy.lst exists)
3. while loop on l.26 is skipped (because hierarchy.lst is empty)
4. if on l.42 is skipped (because FOLDERS is empty)
5. if on l.65 is skipped (because VALUES_FILE is not set) and else on l.67 is entered; hm, so, actually, given set -u at the beginning of the file, I believe we'll have an error in this case.

Given the above (unless I'm misguided), I believe it would be good if we (1) fix handling of empty hierarchy.lst by doing some explicit choice, to make sure it doesn't result in some accidental unexpected code path, and (2) if we want to allow empty/no parameters in the end, prune the dead code fragment on l. 67-69.

Also, notably, I believe our behavior of empty parameters is kinda semi-accidentally drifting. Initially, we seemed to strongly disallow empty params, which was even too strong, as it was breaking hello-world-yaml. However, I guess the idea made some sense for non-trivial template processors (i.e. all except pure eunomia-base), to help detect CR errors. Now, after numerous code changes in this file, we seem to have got to a place, where we de facto allow empty parameters; this makes obvious sense for hello-world-yaml, but I'm not sure if we are sure we want to remove the protection which existed for non-trivial processors. But if we do, I believe we shouldn't keep the never-used error block in the script, as it might be misleading.

What are your thoughts on this? Would you want to take a look at this by yourself, or would you prefer if I open a new separate issue describing this situation?

Thanks!

[vinny-sabatini]

Yeah this is a bit tricker than I thought. My vote would be to create a separate issue where we explicitly determine the logic here/how we want to handle empty vars. Then the issue can include tests to enforce this functionality, and if we ever refactor/reduce our bash footprint in this repo, we can make sure we aren't breaking our expected behavior 😄

If you could create the issue with your current understanding of the logic, that would be greatly appreciated!

[akavel]

Hmm.... so, after some more pondering, I'm starting to think that "tests" is indeed the key word here. I tried to start writing the issue, but without tests, fixing anything here is kinda too fragile anyway; and once we get them, some of this will become much clearer already. So in the end, I think I'll skip for now; it's one of the many papercuts that we'll hopefully have better tools to resolve once we have better tests coverage... Thanks for the discussion! :)