Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a compose ID parser, tweak short and version rules #83

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AdamWill
Copy link
Contributor

This adds a parse_compose_id function to composeinfo. It
can parse most compose IDs back into their component parts. As
part of this, we tweak the rules for short names and versions
a little. The RELEASE_SHORT_RE regex now allows upper-case
characters, as Fedora has been using these in short names for
some time (only create_release_id actually validated short
names, up until now, which is why we hadn't noticed this
inconsistency). The RELEASE_VERSION_RE regex now does not
allow absolutely any string that doesn't start with a digit to
be used as a version; the only string version allowed is
'Rawhide' (this is the only one that exists in the wild so far
as I know). This is to make it possible to write a sane parser
at all.

We also fix get_date_type_respin to work with the new ci
type.

We could possibly go further than this and disallow the use of
dashes in short names; if we did that, parsing compose IDs
would become much less difficult and slightly more reliable.
But it would require us to either rename or 'grandfather in'
Fedora's existing short names with dashes in them, which are
Fedora-Atomic, Fedora-Docker and Fedora-Cloud. It would be
reasonably easy to write a parser and regex which special case
those exact names but disallow any other with a dash.

Signed-off-by: Adam Williamson [email protected]

This adds a `parse_compose_id` function to `composeinfo`. It
can parse most compose IDs back into their component parts. As
part of this, we tweak the rules for short names and versions
a little. The `RELEASE_SHORT_RE` regex now allows upper-case
characters, as Fedora has been using these in short names for
some time (only `create_release_id` actually validated short
names, up until now, which is why we hadn't noticed this
inconsistency). The `RELEASE_VERSION_RE` regex now does *not*
allow absolutely any string that doesn't start with a digit to
be used as a version; the only string version allowed is
'Rawhide' (this is the only one that exists in the wild so far
as I know). This is to make it possible to write a sane parser
at all.

We also fix `get_date_type_respin` to work with the new `ci`
type.

We could possibly go further than this and disallow the use of
dashes in short names; if we did that, parsing compose IDs
would become much less difficult and slightly more reliable.
But it would require us to either rename or 'grandfather in'
Fedora's existing short names with dashes in them, which are
Fedora-Atomic, Fedora-Docker and Fedora-Cloud. It would be
reasonably easy to write a parser and regex which special case
those exact names but disallow any other with a dash.

Signed-off-by: Adam Williamson <[email protected]>
@AdamWill
Copy link
Contributor Author

Note: I considered having the parser return a namedtuple rather than a dict. I slightly dislike dict as a return type for this kind of thing because it's easy to forget that dicts are mutable and the pitfalls of dealing with mutable objects. But I thought I'd go with the core type to start with. If you like the idea of a namedtuple, yell. Note that namedtuples appeared in Python 2.6, so they're available for RHEL 6 but not RHEL 5; dunno if that's a problem.

Please suggest any other compose IDs to test the parser against, this set includes all the ones from the create_compose_id test plus some from fedfind's test suite.

Please also let me know what you think of the 'upper-case characters in short names' issue, and the possibility of disallowing dashes in short names.

@AdamWill
Copy link
Contributor Author

AdamWill commented Mar 2, 2017

Ping? Anything?

@lubomir
Copy link
Contributor

lubomir commented Mar 16, 2017

Think again about this, it seems like a lot of relatively fragile code. If I recall correctly, your use case is consuming data from Fedmsg and you want to avoid having to download the metadata files, right?

If so, what about changing the message and actually putting all this information directly into the message?

@AdamWill
Copy link
Contributor Author

Basically my case is we've been doing this for a while and it's actually easier to write this code than go back through all the various things we do with composes and unpick the ones that find composes by compose ID. If you don't accept the code in productmd I'm more likely to just keep it in fedfind than change all of those things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants