Skip to content

PRDoc new schema#1946

Merged
chevdor merged 38 commits into
masterfrom
wk-231018-prdoc-new-schema
Dec 4, 2023
Merged

PRDoc new schema#1946
chevdor merged 38 commits into
masterfrom
wk-231018-prdoc-new-schema

Conversation

@chevdor

@chevdor chevdor commented Oct 19, 2023

Copy link
Copy Markdown
Contributor

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-no-crate-publish-required The change does not require any crates to be re-published. label Oct 19, 2023
@chevdor chevdor removed the R0-no-crate-publish-required The change does not require any crates to be re-published. label Oct 19, 2023
@chevdor chevdor added the R0-no-crate-publish-required The change does not require any crates to be re-published. label Oct 19, 2023
chevdor added a commit that referenced this pull request Oct 19, 2023
@chevdor chevdor removed the R0-no-crate-publish-required The change does not require any crates to be re-published. label Oct 20, 2023
Comment thread prdoc/schema_user.json Outdated
Comment thread prdoc/schema_user.json
Comment on lines +141 to +156
"migration_db": {
"type": "object",
"properties": {
"name": {
"type": "string"
},
"description": {
"type": "string"
}
},
"additionalProperties": false,
"required": [
"name",
"description"
]
},

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 thread prdoc/schema_user.json
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same.

@chevdor chevdor Oct 23, 2023

Copy link
Copy Markdown
Contributor Author

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: []

Comment thread prdoc/schema_user.json Outdated
tdimitrov pushed a commit that referenced this pull request Oct 23, 2023
@chevdor

chevdor commented Oct 25, 2023

Copy link
Copy Markdown
Contributor Author

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

@bkchr

bkchr commented Oct 25, 2023

Copy link
Copy Markdown
Member
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.

@chevdor chevdor requested review from Morganamilo and bkchr November 21, 2023 08:55
@bkchr

bkchr commented Nov 21, 2023

Copy link
Copy Markdown
Member

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

@chevdor

chevdor commented Nov 21, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

Comment thread docs/CONTRIBUTING.md
@chevdor chevdor requested review from a team and mordamax November 29, 2023 10:29
@chevdor chevdor enabled auto-merge (squash) December 1, 2023 07:37
Comment thread prdoc/schema_user.json
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
Copy Markdown
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
Copy Markdown
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.

@mutantcornholio mutantcornholio left a comment

Copy link
Copy Markdown
Contributor

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
@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.