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

Update service info artifact list to add seqcol and rename refget #48

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tcezard
Copy link
Contributor

@tcezard tcezard commented Feb 21, 2024

The sequence collection specification is currently ongoing GA4GH product approval and as part of this process we're proposing to add a new type to the TASC registry.

The development of the sequence collection specification also saw the rebranding of refget as a more generic term encompassing both refget.sequence (formally refget) and refget.seqcol (for sequence collection)

Copy link
Collaborator

@mamanambiya mamanambiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tcezard for submitting this PR. According the submission guidelines, the entries must be ordered alphabetically. I think refget.seqcol should come before refget.sequence.

@tcezard tcezard requested a review from mamanambiya June 18, 2024 13:56
@mbaudis
Copy link
Member

mbaudis commented Jul 4, 2024

No objections.

@jmarshall
Copy link
Member

jmarshall commented Jul 4, 2024

The proposed artifact values do not conform to the expected kebab case format.

Either TASC should consider widening the expected format e.g. to allow period-based hierarchy in these values (so the regex might be ^([a-z][a-z0-9]*)([.-][a-z0-9]+)*$), or the sequence collection authors should consider using refget-seqcol and refget-sequence.

@mbaudis
Copy link
Member

mbaudis commented Jul 5, 2024

Ouch, right @jmarshall. So the question here is if it is refget with dot indication for a parent.child relation (i.e. description and regex have to be amended) or if they are "independent" refget-seqcol and refget-sequence.

@jmarshall
Copy link
Member

This is the first time we have removed an extant artifact value from this list: here is it refget, which is being renamed. The only process defined in this list's README is for the addition of new items. So we need to think about what the process should be for removing or renaming items.

One purpose of this registry list is to prevent collisions — including collisions involving historical values, which are likely to be still in use by older servers. So IMHO entries should never be removed. In that case we would leave the refget entry as is, adding subfields to the effect of

…, "status": "superseded", "note": "renamed to …whatever… as of …date…", …

Another purpose of this registry is to attest that artifact values are being used appropriately and as per their registry listing. In this case I would expect to see the proposed artifact values listed in the service-info parts of the respective specs, and for the renaming in the sequence protocol I'd expect to see a discussion of older servers returning "type": { "group": "org.ga4gh", "artifact": "refget", … } in their service-info responses.

As far as I can see, the current seqcol specification does not state what the contents of the generic part of the service-info response should be at all. I didn't see any PRs making any additions to specify the proposed value.

For the refget / sequence protocol, the current specification (v2.0) states (by example, not in clearly normative text) that the service-info response should include "artifact": "refget", i.e., the existing artifact value in the registry, prior to this renaming. I didn't see any PRs or similar or other drafts making any changes to the service-info part of the protocol or specification.

(I may have missed seeing the relevant specification drafts. It would be helpful if the opening comment of this PR had pointed to the relevant documents or updates, e.g. as PRs #31 and #41 did. I hope the refget/seqcol authors will do so now.)

In my opinion, until TASC has seen the specification drafts or PRs using the proposed artifact values, we cannot merge these changes to the registry.

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.

5 participants