microbiomedata / nmdc-schema

National Microbiome Data Collaborative (NMDC) unified data model
https://microbiomedata.github.io/nmdc-schema/
Creative Commons Zero v1.0 Universal
27 stars 8 forks source link

Add PR template #1995

Closed kheal closed 3 months ago

kheal commented 4 months ago

We need a PR template to help us decide whether or not PRs can be merged during a 'schema freeze' and helps contributors assign appropriate reviewers.

mslarae13 commented 3 months ago

Would appreciate feedback & suggestions on what should go in / be asked for / check on for the PR template.

@turbomam @naglepuff @eecavanna @shreddd @kheal @corilo @SamuelPurvine @mbthornton-lbl @pkalita-lbl @aclum @sujaypatil96 @brynnz22

kheal commented 3 months ago

One thing to include is specific guidance that walks people through whether or not a PR will need an accompanying migration - and if so, @eecavanna or @brynnz22 should be named to be included in the conversation. From my experience, here are some changes that would likely need migration support 1) slot or Class name changes 2) slot multiplicity changes (from a single value to a list or vice versa) 3) slot movement from one Class to another 4) Enum value changes.

A separate list of "non-breaking" changes people could choose from is also helpful. For example changes to descriptions of slots, classes, or enums; removal of unused, commented, or invalid code; updated mappings of terms, classes, or slots to ontologies; adding an additional Enum to accommodate new or future metadata.

eecavanna commented 3 months ago

Here's the decision tree I use to determine whether a schema change requires a migrator.


Eventually, I want every schema change to be accompanied by a migrator, even if that migrator is a "no op" migrator (i.e. it performs no operations). The presence of that "no op" migrator would serve as documentation that, indeed, no migration was necessary between these schema versions. I don't expect people to be doing this yet, though, as I don't think I've emphasized it sufficiently in communication. So, I'm OK leaving it out of the PR template guidance.

aclum commented 3 months ago

perhaps a section for justification/need for cases where there are downstream consequences

eecavanna commented 3 months ago

Here's what I'm envisioning for the portion of the template related to migrators.

Schema changes and migrators

  1. Does this PR introduce any changes to the schema?
    • [ ] Yes
    • [ ] No
  2. If you answered "Yes" to (1), do those schema changes have the potential to make valid data invalid?
    • [ ] Yes
    • [ ] No
  3. If you answered "Yes" to (2), does this PR include a migrator that makes all valid data remain valid?
    • [ ] Yes
    • [ ] No