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

PRDoc new schema #1946

Merged
merged 38 commits into from
Dec 4, 2023
Merged

PRDoc new schema #1946

merged 38 commits into from
Dec 4, 2023

Conversation

chevdor
Copy link
Contributor

@chevdor chevdor commented Oct 19, 2023

Overview

This PR brings in the new version of prdoc v0.0.6 and allows:

  • local schema
  • local config
  • local template

It also fixes the existing prdoc files to match the new schema.

todo

  • add a brief doc/tldr to help contributors get started
  • test CI
  • finalize schema
  • publish the next prdoc cli version (v0.0.7 or above)

- add config
- add schema
- add template
- fix existing prdocs
@chevdor chevdor requested review from a team as code owners October 19, 2023 15:21
@chevdor chevdor added the T10-tests This PR/Issue is related to tests. label Oct 19, 2023
@chevdor chevdor added the R0-silent Changes should not be mentioned in any release notes label Oct 19, 2023
@chevdor chevdor removed the R0-silent Changes should not be mentioned in any release notes label Oct 19, 2023
@chevdor chevdor added the R0-silent Changes should not be mentioned in any release notes label Oct 19, 2023
chevdor added a commit that referenced this pull request Oct 19, 2023
@chevdor chevdor removed the R0-silent Changes should not be mentioned in any release notes label Oct 20, 2023
prdoc/schema_user.json Outdated Show resolved Hide resolved
Comment on lines 141 to 156
"migration_db": {
"type": "object",
"properties": {
"name": {
"type": "string"
},
"description": {
"type": "string"
}
},
"additionalProperties": false,
"required": [
"name",
"description"
]
},
Copy link
Member

Choose a reason for hiding this comment

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

These should be optional parts of the individual audiences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you are talking about the rendering, this is OK, it will be done in the templates and we can define what audiences will see the migration_db info. That can be done for one or several audiences.

If you refer to moving migration_db inside a specific or multiple audiences in the schema, that will not be very DRY and require providing this information duplicated in some cases.
It is also much easier when editing the prdoc to provide this property close to the top level than down some audiences and have users remember which audience(s) accepts this property.

Copy link
Member

Choose a reason for hiding this comment

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

What will you do with this information that there is a db migration?

A proper prdoc would mention this migration in the proper audience any way to tell the user that it is there. If you only want this for some kind of filtering, fine. However then just add it as optional fields to audience and just a bool to indicate that there is either a db migration or a runtime migration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The prdoc just holds the information, it does not know how we use it. The prdoc file does notthing.

During the release, all the prdoc files involved in the release will be fed into one or more templates (tbd).
Those will define what we actually do with the informations.

For the example with migration_db, this will be shown to some audiences.
@kianenigma gave some more examples here. For the migration_db, the information will be shown to the Runtime dev only.

Comment on lines 203 to 226
"host_function": {
"type": "object",
"additionalProperties": false,
"description": "List of host functions and their properties",
"properties": {
"name": {
"type": "string"
},
"enabled": {
"type": "boolean"
},
"description": {
"type": "string"
},
"notes": {
"type": "string"
}
},
"required": [
"name",
"enabled",
"description"
]
}
Copy link
Member

Choose a reason for hiding this comment

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

Same.

Copy link
Contributor Author

@chevdor chevdor Oct 23, 2023

Choose a reason for hiding this comment

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

Same comment than for the migration_db .

What could be done however if that makes it easier is to not have db and runtime embeded under a migrations section but leave it flat. It may be even easier this way.

So instead of:

migrations:
   db: []
   runtime: []

we could have:

db_migrations: []
runtime_migrations: []

prdoc/schema_user.json Outdated Show resolved Hide resolved
tdimitrov pushed a commit that referenced this pull request Oct 23, 2023
@chevdor
Copy link
Contributor Author

chevdor commented Oct 25, 2023

@bkchr is the current schema in an acceptable state to get started ?

@bkchr
Copy link
Member

bkchr commented Oct 25, 2023

doc:
  - audience: Node Dev
    description: |
      ...
    db_migration: true
    runtime_migration: true
    host_function: true

db_migration, runtime_migration and host_function should be optional. It makes no sense to have them on the "global" level or then somewhere else decide to which audience we want to show these. I also only see these as "information" for further tooling, because I would already expect that the Runtime Dev description mentions the migration and how to use it etc. So, these extra fields are just for better machine processing of these information.

@bkchr
Copy link
Member

bkchr commented Nov 21, 2023

@chevdor then please remove the semver field and I will approve it.

@chevdor
Copy link
Contributor Author

chevdor commented Nov 21, 2023

Ok, I removed the semver field and fixed the existing prdocs.

@chevdor chevdor requested a review from a team November 23, 2023 15:25
@the-right-joyce
Copy link
Contributor

wen merge?

@chevdor chevdor enabled auto-merge (squash) November 27, 2023 11:33
auto-merge was automatically disabled November 27, 2023 15:37

Merge queue setting changed

@chevdor chevdor requested review from mordamax and a team November 29, 2023 10:29
@chevdor chevdor enabled auto-merge (squash) December 1, 2023 07:37
Comment on lines +101 to +103
{"const": "Node Operator",
"title": "Node Operator",
"description": "Those who don't write any code and only run code."},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intended that there is no "Validator" option? We would just choose "Node Operator" for that case?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Most of the changes are equally for both groups and normal node operators are also able to just skip the one validator message for every 5th release.

Copy link
Contributor

@mutantcornholio mutantcornholio left a comment

Choose a reason for hiding this comment

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

🔥

@chevdor chevdor merged commit 756a12d into master Dec 4, 2023
128 of 130 checks passed
@chevdor chevdor deleted the wk-231018-prdoc-new-schema branch December 4, 2023 14:25
juangirini pushed a commit that referenced this pull request Dec 4, 2023
## Overview

This PR brings in the new version of prdoc v0.0.6 and allows:
- local schema
- local config
- local template

It also fixes the existing prdoc files to match the new schema.

## todo

- [x] add a brief doc/tldr to help contributors get started
- [x] test CI
- [x] finalize schema
- [x] publish the next `prdoc` cli version (v0.0.7 or above)

---------

Co-authored-by: Egor_P <egor@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T10-tests This PR/Issue is related to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.