Fix ubi config validation doesn't really validate

[JayZ12138]

Previously, the keyword 'properities' was missing in the config schema,
while the config schema itself was still valid, it wasn't really used to
validate any incoming data. Fix #29

Add 'packages' and 'content_sets' requirements to json schema, which would
fail the validation if those two blocks of configuration are missing. Fix #28

Set additionalProperites to False, so only the declared properities
are allowed.

Also, make module's stream allows both number and string

[rohanpm]

Could we please have some updates to testcases as well? That was just as big a problem here - that validation was effectively a no-op and yet no autotest failed. It really ought to be fixed as well.

[rohanpm]

In the commit message, looks like " [fix #28 #29]" is not going to work to close both issues. The UI is indicating that it only close issue 28.

Could you please adjust it in some way so it closes both? I know in our internal projects we used [] in first line of commits, but I don't think it necessarily makes sense here, probably mentioning the two issues in two separate lines of the commit message body makes more sense.

[rbikar]

I also think we shouldn't just crash when yaml file is invalid or has wrong syntax. Imho in case of invalid file, ubi-config should log error and skip current file from processing at least when we read all config files from remote repo. @JayZ12138 What do you think?

            try:
                config_dict = yaml.safe_load(response.content)
            except yaml.YAMLError:
                LOG.error('There is syntax error in your config file %s, please fix', file_name)
 >>>               raise
        else:
            if self.local_repo:
                file_path = os.path.join(self.local_repo, file_name)
            else:
                file_path = file_name
            LOG.info("Loading configuration file locally: %s", file_path)
            with open(file_path, 'r') as f:
                config_dict = yaml.safe_load(f)

        # validate input data
 >>>       validate_config(config_dict)

[JayZ12138]

In the commit message, looks like " [fix #28 #29]" is not going to work to close both issues. The UI is indicating that it only close issue 28.

Could you please adjust it in some way so it closes both? I know in our internal projects we used [] in first line of commits, but I don't think it necessarily makes sense here, probably mentioning the two issues in two separate lines of the commit message body makes more sense.

Hmm, just feel [] makes it more clear.
I'll change it

[JayZ12138]

Could we please have some updates to testcases as well? That was just as big a problem here - that validation was effectively a no-op and yet no autotest failed. It really ought to be fixed as well.

I was thinking about the test cases, but seems like there's no way to verify it really validate the incoming config.
So, I guess the only possible test is to feed it with invalid configs and check if the right exception is raised.

[JayZ12138]

I also think we shouldn't just crash when yaml file is invalid or has wrong syntax. Imho in case of invalid file, ubi-config should log error and skip current file from processing at least when we read all config files from remote repo. @JayZ12138 What do you think?

            try:
                config_dict = yaml.safe_load(response.content)
            except yaml.YAMLError:
                LOG.error('There is syntax error in your config file %s, please fix', file_name)
 >>>               raise
        else:
            if self.local_repo:
                file_path = os.path.join(self.local_repo, file_name)
            else:
                file_path = file_name
            LOG.info("Loading configuration file locally: %s", file_path)
            with open(file_path, 'r') as f:
                config_dict = yaml.safe_load(f)

        # validate input data
 >>>       validate_config(config_dict)

@rbikar
hmm. I'm not sure, as James once said, it's produced by machine, so I feel it's not so possible only one file contains error, it's probably None or all.

But, yeah, from our point of view, it's suitable to do this change. I'll change it anyway.

[rbikar]

But

We had some invalid yaml files in one branch, now it's fixed. It had empty packages whitelist.

[midnightercz]

I think tests should cover more possibilities of incorrect input.

[JayZ12138]

@midnightercz
Hm, yeah, should be more.
But, I also think the test cases here are used to test if some specific keywords worked or not, such as 'additionalProperties', 'required', 'properties', instead of covering every line, which is too much and not necessary.
'additionalProperties' and 'required' has been tested, so I guess we can further add test cases for 'type', do you think so?

[rohanpm]

we shouldn't just crash when yaml file is invalid or has wrong syntax. Imho in case of invalid file, ubi-config should log error and skip current file from processing at least when we read all config files from remote repo

I think this should be "at most", as it's unnecessarily more difficult to write correct code if load("somefile.yml") is defined as simply returning None if there was any problem with somefile.yml.

Either way, it seems to be a separate issue from the validation not working.

[JayZ12138]

we shouldn't just crash when yaml file is invalid or has wrong syntax. Imho in case of invalid file, ubi-config should log error and skip current file from processing at least when we read all config files from remote repo

I think this should be "at most", as it's unnecessarily more difficult to write correct code if load("somefile.yml") is defined as simply returning None if there was any problem with somefile.yml.

Either way, it seems to be a separate issue from the validation not working.

What I think is, if there's such error in config file, then we should file a ticket to the team who generated it and ask them to resolve this immediately. If we really want to continue the push, we can enter the config files one by one manually.

I filed a separately issue already.

[rohanpm]

I feel like this isn't testing what you wanted, since the test name says "wrong data type" but here you've got a typo 'inlcude'. Did you mean 'include' so that you'd test what happens if include is a string?

[JayZ12138]

yeah, it's a typo here..

[midnightercz]

'additionalProperties' and 'required' has been tested, so I guess we can further add test cases for 'type', do you think so?

I think for every key that is required to be in some form (value, type, non-empty) by schema. We should have following set of tests:

• test wrong type
• test key missing
• test key empty (None)
• test key correct
  You can parametrize those tests to avoid creating duplicate tests

[JayZ12138]

'additionalProperties' and 'required' has been tested, so I guess we can further add test cases for 'type', do you think so?

I think for every key that is required to be in some form (value, type, non-empty) by schema. We should have following set of tests:

• test wrong type
• test key missing
• test key empty (None)
• test key correct
  You can parametrize those tests to avoid creating duplicate tests

Hmm, I feel it's too complicated.
I think the functional tests should be enough to make sure the schema is working as we expected.
It's like we make unit test for some function, make sure it's working as expected, then in the integration test, we mock it instead of testing it repeatedly.
Now we have prove the keywords and type are working, why do we test them repeatedly?

[midnightercz]

Now we have prove the keywords and type are working, why do we test them repeatedly?

But you don't prove tat keywords and types are working. I believe that's covered in unit tests in json-schema library.
You need to test that your schema is correct. By that I mean that you have correct restrictions in correct places