Skip to content

Commit

Permalink
Merge pull request #291 from lsst/tickets/DM-48013-jeremym
Browse files Browse the repository at this point in the history
DM-48013: Add contribution guide and pull request template
  • Loading branch information
JeremyMcCormick authored Dec 17, 2024
2 parents 43bd5d1 + ef1d20b commit c1fc04d
Show file tree
Hide file tree
Showing 4 changed files with 125 additions and 0 deletions.
6 changes: 6 additions & 0 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
## Checklist

When making changes to YAML files in the [schemas](../python/lsst/sdm_schemas/schemas) directory:

- [ ] If applicable, incremented the schema version number, following the guidelines in the [contribution guide](../CONTRIBUTING.md)
- [ ] Referred to the [documentation on specific schemas](../CONTRIBUTING.md#specific-schema-documentation) for additional versioning information, change constraints, or tasks that may need to be performed, based on which schema is being updated
70 changes: 70 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
Contributing to SDM Schemas
===========================

Creating a PR
-------------

All changes to the repository must be made by [opening a pull request](https://github.com/lsst/sdm_schemas/compare), which will require one approving reviewer.
Any branches used for PRs which contain non-trivial changes should follow the [standard DM naming convention for ticket branches](https://developer.lsst.io/work/flow.html#ticket-branches), or the PR will be rejected and closed.

The PR title should be formatted like: `[TICKET]: [TITLE]` where `TICKET` is the Jira ticket number and `TITLE` is a short description.

For example, this is a valid PR title, with a dummy ticket number:

`DM-12345: Add ra and dec columns to test schema`.

Including the name of the schema in the PR title is also helpful for the reviewer and provenance, e.g., `test` in this case.

An appropriate reviewer should be assigned, based on which schema is being changed or added.
If the author does not know who should review their PR, [JeremyMcCormick](https://github.com/JeremyMcCormick) can be assigned as a default, and an appropriate reviewer will be found.

The author should [resolve the review](https://developer.lsst.io/work/flow.html#resolving-a-review) if changes are requested.
Once the PR is approved, it may then be merged by the author.

Pull Request Checks
-------------------

SDM Schemas pull requests have an extensive set of checks that will run automatically.
Aside from special cases, a pull request will have to pass all of these checks before it can be merged.

These checks will:

- [Build TAP_SCHEMA and DataLink resources](.github/workflows/build.yaml)
- [Compare Schemas for Changes](.github/workflows/compare.yaml) - Schemas that are in the [deployed list](./python/lsst/sdm_schemas/schemas/deployed-schemas.txt) will fail this check.
- [Build the Schema Browser website](.github/workflows/docs.yaml)
- [Check that `main` is not merged into the branch](.github/workflows/rebase_checker.yaml)
- [Test that the schemas can be used to create databases](.github/workflows/test_databases.yaml)
- [Vaildate the schemas using Felis](.github/workflows/validate.yaml)
- [Lint YAML files](.github/workflows/yamllint.yaml)

The checks will rerun anytime the PR branch is updated.

Schema Versioning
-----------------

Individual schemas may have their own internal [version](https://felis.lsst.io/user-guide/model.html#schema-version) for tracking changes, which is defined at the top of the file, as in [apdb.yaml](./python/lsst/sdm_schemas/schemas/apdb.yaml).
(This is distinct from tags or versions of sdm_schemas itself, and not all schemas may be using them.)
This version may need to be updated when making changes.

It is recommended to use versions with three numbers in the format `MAJOR.MINOR.PATCH`, such as `1.2.3`, though any scheme may be used in practice.

The following guidelines can be used for incrementing the version:

- Different `MAJOR` versions are incompatible.
- Different `MINOR` versions are backward compatible.
- Different `PATCH` versions are completely compatible.

These suggestions need not be followed strictly, and there may be exceptions.
For instance, some operations which are technically backward-compatible, such as adding a table, should likely trigger a `MAJOR` version increment rather than `MINOR`, as they would constitute an important and significant update, potentially requiring significant changes to client code or APIs.

The [specific schema documentation](#specific-schema-documentation) should be consulted for more specific rules and guidelines on version incrementing.

Database Migrations
-------------------

Database migrations are currently performed with schema-specific tooling in external repositories, so documentation for that particular schema should be consulted on how to perform them and how this may impact the pull request process.

Specific Schema Documentation
-----------------------------

- [Alert Production Database (APDB)](docs/APDB.md)
49 changes: 49 additions & 0 deletions docs/APDB.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
Alert Production Database (APDB) Schema
=======================================

The [apdb](../python/lsst/sdm_schemas/schemas/apdb.yaml) schema describes the Alert Production Database (APDB), which contains the results of processing from the [Alert Production Pipeline](https://github.com/lsst/ap_pipe).

Schema Versioning
-----------------

The versioning system of the APDB is outlined in [DMTN-269](https://dmtn-269.lsst.io/).
The deployed database schema is a product of the [schema in sdm_schemas](../python/lsst/sdm_schemas/schemas/apdb.yaml) with additional processing by the APDB client library from the [dax_apdb](https://github.com/lsst/dax_apdb) repository.
Compatibility of the client executable with the actual database schema is determined by the version number in the YAML file and the version stored in the database metadata.

Backward compatibility in the context of the APDB implies that a client using a schema with a higher minor version will be able to read and write into a database with a lower minor version -- in other words, an existing database instance need not be migrated to match the newer client interface.
But a client using a schema with a lower minor version will be incompatible with a database that has a higher version number.
There may be cases where significant schema changes may still be compatible if the client can handle them transparently.

Typical Schema Changes
----------------------

Here are some guidelines for incrementing the version number based on what changes are being performed:

| Operation | Increment | Notes |
|--------|-----------|-------|
| Add table | `MAJOR` | The client using the new schema will typically want to read or write into that table, which will make it incompatible.|
| Add column | `MAJOR` | The client using the new schema will typically want to read or write into that column, which will make it incompatible.|
| Remove column | `MINOR` or `PATCH` | Clients using the new schema will not care about a removed column, so this should be backward compatible.|
| Change column type | `MINOR` or `PATCH` | This may or may not be completely transparent and so could be either backward compatible or fully compatible.
| Add index | `PATCH` | This should be fully backward compatible.|
| Change object metadata | - | This does not affect the database schema and should not require a version change.|

Client Compatibility
--------------------

Changes to the APDB schema must be propagated to the Avro alert schemas defined in the [alert_packet](https://github.com/lsst/alert_packet) repository, following the instructions under [Adding a new schema](https://github.com/lsst/alert_packet/tree/main?tab=readme-ov-file#adding-a-new-schema).

Database Migrations
-------------------

The APDB database schema is upgraded using a special migration tool defined in the [dax_apdb_migrate](https://github.com/lsst-dm/dax_apdb_migrate) repository.
For every version change to `apdb.yaml`, a new migration script needs to be added to that repository.
The details of the migration process are described in the [package documentation](https://github.com/lsst-dm/dax_apdb_migrate/blob/main/doc/lsst.dax.apdb_migrate/index.rst).
The APDB includes a Cassandra-based implementation in addition to a SQL-based one, and the Cassandra implementation may impose additional restrictions on schema changes.
Migration scripts should normally be implemented on the same Jira ticket as the corresponding schema change.

Reviewers
---------

- [Ian Sullivan](https://github.com/isullivan) - primary reviewer (may reroute to another reviewer depending on changes)
- [Andy Salnikov](https://github.com/andy-slac) - for guidance on updating schema versions
File renamed without changes.

0 comments on commit c1fc04d

Please sign in to comment.