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

DM-48013: Add contribution guide and pull request template #291

Merged
merged 4 commits into from
Dec 17, 2024

Conversation

JeremyMcCormick
Copy link
Collaborator

@JeremyMcCormick JeremyMcCormick commented Dec 9, 2024

Added:

  • Contribution guide at the standard location of CONTRIBUTING.md
  • New pull request template with:
    • Checkbox for incrementing the schema version number, if applicable
    • Checkbox for referring to schema-specific information that may need to be consulted
  • APDB documentation under docs/APDB.md, which includes versioning recommendations and other information relevant to contributing

It is planned to add more schema-specific documentation under docs for all major schemas, but probably not on this PR. Documentation for APDB is included here as a working case for which information to put in the main guide and what to put under the schema-specific pages.


The new contribution guide can be previewed here.


An APDB-specific schema versioning guide by @andy-slac which was used for the content in docs/APDB.md can be found here.

@JeremyMcCormick JeremyMcCormick marked this pull request as draft December 9, 2024 21:19
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@JeremyMcCormick
Copy link
Collaborator Author

JeremyMcCormick commented Dec 9, 2024

@kfindeisen I added a short section in the APDB docs mentioning that changes must be propagated to alert_packet. If you have specific language that you would like to include there, please let me know.

In terms of PR mechanics, does a merged PR on apdb.yaml trigger the updates to api_package, which could then come later? Or is this supposed to be done as a more-or-less simultaneous update to both repos?

@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-48013-jeremym branch 5 times, most recently from 647684e to 6d989d7 Compare December 9, 2024 22:58
@kfindeisen
Copy link
Member

@kfindeisen I added a short section in the APDB docs mentioning that changes must be propagated to alert_packet. If you have specific language that you would like to include there, please let me know.

I have only a very rough knowledge of alert_packet; please direct those questions to @ebellm.

@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-48013-jeremym branch 8 times, most recently from cbcb39c to 0b77c7f Compare December 9, 2024 23:50
@JeremyMcCormick JeremyMcCormick marked this pull request as ready for review December 9, 2024 23:52
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-48013-jeremym branch 2 times, most recently from 967ec20 to b3c04e2 Compare December 10, 2024 00:03
CONTRIBUTING.md Outdated Show resolved Hide resolved
@JeremyMcCormick JeremyMcCormick added the documentation Improvements or additions to documentation label Dec 10, 2024
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-48013-jeremym branch 3 times, most recently from e113f42 to 2888a45 Compare December 10, 2024 19:22
@JeremyMcCormick JeremyMcCormick marked this pull request as draft December 10, 2024 19:39
Copy link
Contributor

@andy-slac andy-slac left a comment

Choose a reason for hiding this comment

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

I think there is a lot of confusion about the role of the version numbers. I agree with Krzysztof that we should stop calling it SemVer. I think people used to think about version in terms of APIs, where "server" side can be updated before the "client" side (e.g. dynamic library is updated, which can cause rebuilding of the client apps). In our case the roles are reversed - we update sdm_schemas which is "immediately" picked up by the database client code, and only later we can decide to upgrade the actual database schema.

One more thing that I believe is implied in your description of schema compatibility is that the database schema version should be tracked somewhere (in the database itself), but there is no explicit mentioning of that anywhere.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/APDB.md Outdated Show resolved Hide resolved
docs/APDB.md Outdated Show resolved Hide resolved
docs/APDB.md Outdated Show resolved Hide resolved
docs/APDB.md Outdated Show resolved Hide resolved
@JeremyMcCormick
Copy link
Collaborator Author

JeremyMcCormick commented Dec 11, 2024

@andy-slac

I agree with Krzysztof that we should stop calling it SemVer.

Based on this feedback, I reworded this section and removed the part about using using semantic versioning. Please have a look at the updated text.

we update sdm_schemas which is "immediately" picked up by the database client code, and only later we can decide to upgrade the actual database schema

I'm still not sure if this is a general case or APDB-specific. I'm going to make the main contributing text more agnostic about it.

the database schema version should be tracked somewhere (in the database itself), but there is no explicit mentioning of that anywhere

I plan to add this capability - see DM-47340.

@JeremyMcCormick
Copy link
Collaborator Author

JeremyMcCormick commented Dec 11, 2024

@kfindeisen

Thank you for the thorough review!

I think this needs more coordination (with @andy-slac?) over what changes actually cause breakages. In particular, the primary table of examples assumes that DBs always use the latest schema, which in my experience is not true (migration support is often a low priority).

After discussing and reading through comments, I have realized that using an updated client on an older schema version is a more typical case than the reverse, so I will rewrite the main versioning "operations" table accordingly.

I also found quite a few typos and broken links, but I don't expect any controversy there.

The ones you mentioned should all be fixed now. Please let me know if you find additional mistakes.

@JeremyMcCormick
Copy link
Collaborator Author

JeremyMcCormick commented Dec 11, 2024

w.r.t.

In particular, the primary table of examples assumes that DBs always use the latest schema, which in my experience is not true (migration support is often a low priority).

and

I think people used to think about version in terms of APIs, where "server" side can be updated before the "client" side (e.g. dynamic library is updated, which can cause rebuilding of the client apps). In our case the roles are reversed - we update sdm_schemas which is "immediately" picked up by the database client code, and only later we can decide to upgrade the actual database schema.

I don't think we can assume this as a general use case, as some other schema owners are considering it from the other direction.

I have made the main contributing guide a lot more generic, assuming that the schema subpages will outline their own versioning guidelines. I have also removed the table from CONTRIBUITNG which was providing suggestions for specific operations like removing or adding a column.

@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-48013-jeremym branch 5 times, most recently from 223e8f2 to 7e76e25 Compare December 11, 2024 23:34
docs/APDB.md Outdated Show resolved Hide resolved
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-48013-jeremym branch 5 times, most recently from 50d951c to 040a445 Compare December 12, 2024 00:51
docs/APDB.md Outdated Show resolved Hide resolved
Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

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

Looks much better, thanks! Just minor comments.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-48013-jeremym branch 2 times, most recently from 7505866 to 562e02f Compare December 16, 2024 18:41
Copy link
Contributor

@andy-slac andy-slac left a comment

Choose a reason for hiding this comment

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

Looks good, few suggestions.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/APDB.md Outdated Show resolved Hide resolved
docs/APDB.md Outdated Show resolved Hide resolved
docs/APDB.md Outdated Show resolved Hide resolved
docs/APDB.md Outdated Show resolved Hide resolved
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-48013-jeremym branch 2 times, most recently from 7036b05 to 68cbe4d Compare December 16, 2024 22:14
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-48013-jeremym branch from 68cbe4d to ef1d20b Compare December 16, 2024 22:18
@gpdf gpdf self-requested a review December 17, 2024 00:15
Copy link
Collaborator

@gpdf gpdf left a comment

Choose a reason for hiding this comment

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

Looks good now. Don't wait for schema-specific documentation for ConsDB, but let's make sure there's a ticket for adding that.

@JeremyMcCormick JeremyMcCormick merged commit c1fc04d into main Dec 17, 2024
10 checks passed
@JeremyMcCormick JeremyMcCormick deleted the tickets/DM-48013-jeremym branch December 17, 2024 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants