PEP-440 Not supported by EventMetadata

[Nekenhei]

Description

sourcelib field of EventMetada does not support PEP-440 post, pre and development releases due to intended parse of version data to integer. @hooks-extension-framework

Context

During the development process of a new signal we face the need to create a fork of the v9.10.0 version of openedx-events package. To properly follow the versioning protocol, we decide to use post releases for the mentioned version so in that way we won't interfere with the upstream versioning in a case of syncing the changes.

Once the post release v9.10.0.post1 was released, we face an issue with our CI process: openedx-events eventmetada raised an error due to trying to parse an string value into integer.

https://packaging.python.org/en/latest/discussions/versioning/#valid-version-numbers

This was caused by the "post1" key set in the version definition of the package.

Solution

Remove the map function that tries to parse the version keys to integer. and let the field defined this way:

    sourcelib = attr.ib(
        type=tuple, default=None,
        converter=attr.converters.default_if_none(
            attr.Factory(lambda: tuple(openedx_events.__version__.split(".")))
        ),
        validator=attr.validators.instance_of(tuple),
    )

We already implement this solution in our fork and no test nor workflows were impacted.

[feanil]

@openedx/committers-openedx-events can someone take a look at this an see if it makes sense to apply the change here or maybe even further upstream?

[mariajgrimaldi]

Thank you for reporting this!

According to the OEP-47, we're currently not complying with the local version for forks so we should make the necessary changes to do so. My main concern is whether we can maintain backward compatibility - is this possible? What would be the impact of this change for event consumers?

@Nekenhei: Can you help us open a PR with the change and go over my concerns? I'd really appreciate it. Thank you!

[mariajgrimaldi]

^ My concern could be addressed by simply adding this change as a breaking change so the library is now compliant, letting consumers know that one of the fields changed their type.

FYI @robrap @timmc-edx

[robrap]

I think a breaking change is fine as long as we avoid any known backward incompatibilities. This might take an expand/contract, or it might be even simpler.

Looking at event-bus-kafka, it seems this parsing to int code would need to change first. I don't think anything is relying on these being ints, but that would take a deeper review and a PR to remove that. I'm guessing that the data sent across the bus has the concatenated string (needs to be verified), and thus should be the same either way. In any case, the dependent libraries will need to be updated first.

[navinkarkera]

Looking at event-bus-kafka, it seems this parsing to int code would need to change first.

Just noting that event-bus-redis also does the same, i.e. parse to int

[mariajgrimaldi]

We could first update both backends to support both approaches, merge that, and check it works. Then we merge this change, test again, and finally remove the extra checks. If anything breaks along the way, we revert, fix the issues, and retry. Since the backends already support both approaches, we wouldn’t need more updates at that point. What do you think?