Inform caller if web_catatog_only key is missing

[mgoerens]

Add a "success" return value to get_web_catalog function to inform the caller if the file (OWNERS or report.yaml) doesn't contain such a key.

While this is currently not used by callers, this is a pre-requisite for #344 to exit draft status

[mgoerens]

That said, I'm having trouble understanding why we would need to know if the key existed or not. I reviewed the draft PR this blocks, and It wasn't quite clear to me what decisions we're making where we actually care if the value exists vs. the meaning of the value, which we already get back from this function.

This is were where I check for the value and its existence in the OWNERS: https://github.com/openshift-helm-charts/development/pull/344/commits/d67c95850a85e020e566d489747d7dc3a44d5733#diff-6cdd6c8509751e3be6c04f9c38d4c43c16760aab0318c9ed6cbad88bc9b89300R341-R345

And similarly, a few lines later, in the report: https://github.com/openshift-helm-charts/development/pull/344/commits/d67c95850a85e020e566d489747d7dc3a44d5733#diff-6cdd6c8509751e3be6c04f9c38d4c43c16760aab0318c9ed6cbad88bc9b89300R354-R360

The problem is that the function previously defaults to returning False. In the event the key is not present in OWNERS, I want to return an error.

I made the assumption that this key is mandatory, both in OWNERS and in report.

[komish]

@mgoerens For the purposes of certification, however, a value of "False" (whether the key is found and set to false, key is found and set to null, or key is not found) is still a valid value, right?

The linked logic seems to intend to parse a given submission, and populate the CI environment with useful representations of the project's context. E.g. "This is a webCatalogOnly project; This is a Partner project". For that purpose, the existing logic makes sense. I don't know if we want to raise errors for OWNERS file content on chart submissions when the format of the OWNERS file could be validated in other contexts (e.g. when submitted).

Does that make sense? Is your intention to use this Submission class for every PR, regardless of its purpose?

[EDIT] I know the linked logic isn't in this PR, so I don't want to derail - but the only reason I mention it is because the linked logic drives the need for this PR. I very much appreciate you partitioning these two things out into separate PRs.

[mgoerens]

@komish Sorry, took some time before coming back to this.

Is your intention to use this Submission class for every PR, regardless of its purpose?

I believe we have 2 types of PRs: ones that add / modify an OWNERS file, and one that adds a chart. I try to design the Submission class in a way that is can be initialized in both cases (calling my_submission = Submission(api_url=...) then uses methods is_valid_owners_submission() and is_valid_certification_submission() to check.

However this particular method is only meant to be called on "Certification" type of Submissions.

Could be designed differently, I'm thinking of using Subclasses. Having is_valid_... type of methods also makes it easier to write unit tests BTW. But I'm digressing.

I don't know if we want to raise errors for OWNERS file content on chart submissions when the format of the OWNERS file could be validated in other contexts (e.g. when submitted).

It should definitely be validated when submitted. But still... what if it's somehow absent ? There isn't really a good default value for web_catalog_only. It feels odd to me to default to False if absent. Generally speaking, what to do with a Chart submission if the OWNERS is of invalid format ? I'd rather error the certification.

Note that I will address your comments reg. code once we settle the discussion here. I don't want a bunch of linked PRs to clutter the GitHub UI ... you know ...

[komish]

Hey @mgoerens! I'm just getting back to this, sorry for the delay - let's finish this up so we can get it merged in.

It should definitely be validated when submitted. But still... what if it's somehow absent ? There isn't really a good default value for web_catalog_only.

I suppose this is a valid point. To a certain extent, for simpler logic like these lookups, the library is trying to "tell you the information you want when calling the function", which is - if it's missing, then it is not explicitly set to "true" and therefore should return "false" - and I tend to lean this way.

With that said, I think we could do a couple of things here to get the best of both. LMK how you feel about these options

1) Optionally Raise an exception if the key is missing, e.g.

# pseudocode
def get_web_catalog_only(owner_data, raise_if_missing=False):
    success = False

    #
    # ... code omitted for brevity ...
    #

    if not success and raise_if_missing:
        raise(ConfigKeyMissing("Neither web_catalog_only nor providerDelivery keys were set"))

    return owners_web_catalog_only or owners_providerDelivery

Doesn't change the return, and should allow you to catch ConfigKeyMissing (which is a made up exception not defined in the snippet) in cases where you want to return an error. You don't have to pass the ConfigKeyMissing as the final exception, but catching it should allow you to return whatever you want in cases where you're validating.

2) Use the get method on maps to resolve the "unset" nature of the web_catalog_only and/or providerDelivery keys. E.g.

(owners_file.get('web_catalog_only', 'unset') == 'unset') and (owners_file.get('providerDelivery', 'unset') == 'unset')

[mgoerens]

@komish

I like the idea of using raise_if_missing a lot. That allows for the best of both world indeed.

I'm not sure I follow the second idea though. What does this solve ?

[komish]

The second option just pushes the responsibility of checking whether or not the key is set to the caller of the original function at call time. It's much longer, much more verbose, but leaves the original functions intact and accomplishes the same thing.