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.

Sign up for GitHub

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

Jump to bottom

report: add report versioning #28121

Merged
merged 1 commit into from
Jun 20, 2019
Merged

report: add report versioning #28121

merged 1 commit into from
Jun 20, 2019

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jun 7, 2019

Marking as WIP, as this is still being discussed.

This commit adds a semver version to the diagnostic report feature.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@cjihrig cjihrig added the report Issues and PRs related to process.report. label Jun 7, 2019
@nodejs-github-bot
Copy link
Collaborator

Sadly, an error occurred when I tried to trigger a build. :(

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 7, 2019
@cjihrig cjihrig mentioned this pull request Jun 7, 2019
Closed
@cjihrig cjihrig changed the title report: add report versioning WIP: report: add report versioning Jun 7, 2019
@boneskull
Copy link
Contributor

damn if it ain't the flash over here. that was quick

boneskull
boneskull reviewed Jun 7, 2019
View reviewed changes
doc/api/report.md Outdated
@@ -22,6 +22,7 @@ is provided below for reference.
```json
{
"header": {
"reportVersion": "1.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless we need major.minor.patch or major.minor for some reason, "reportVersion": 1 would be sufficient. How would minor/patch versions help here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally like the idea of a single number, but it was brought up in the original issue. Easy to change it once we figure out what we want to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a lot of skin in this game, opinions of consumers (like @boneskull ) should weigh more than mine. That said, minor would be additions, like new sections, new keys, etc. major would be backwards incompatible changes. and patch... not sure, can there be a bug fix? I guess, maybe, in theory. That NAPI and ABI are versioned with single numbers though makes me think maybe this should do likewise.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for consistency with N-API and ABI versioning.

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if some guidelines were written down as to when to bump the version and when not to bump the version. Some of the keys in the sections are obtained by looping through structures and could therefore have keys added to the report as a result of changes elsewhere in the codebase without any of the report code changing.

@cjihrig cjihrig changed the title WIP: report: add report versioning report: add report versioning Jun 20, 2019
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/23990/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/24009/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/24010/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/24014/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 20, 2019 •
edited by cjihrig
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/24018/

EDIT(cjihrig): CI was yellow.

@cjihrig
report: add report versioning
bfe4c9c 
This commit adds a version to the diagnostic report feature.

PR-URL: nodejs#28121
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@cjihrig cjihrig merged commit bfe4c9c into nodejs:master Jun 20, 2019
@cjihrig cjihrig deleted the report-version branch June 20, 2019 20:18
@boneskull
Copy link
Contributor

🎉

targos pushed a commit that referenced this pull request Jul 2, 2019
@cjihrig @targos
report: add report versioning
c4a357d 
This commit adds a version to the diagnostic report feature.

PR-URL: #28121
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@targos targos mentioned this pull request Jul 2, 2019
Merged
This was referenced Jul 3, 2019
Closed
Closed
Closed
@mmarchini mmarchini mentioned this pull request Jul 8, 2019
Closed
@richardlau richardlau mentioned this pull request Jan 17, 2020
Closed
4 tasks
@gireeshpunathil gireeshpunathil mentioned this pull request May 30, 2022
Merged
4 tasks
gireeshpunathil added a commit to gireeshpunathil/node that referenced this pull request Oct 18, 2022
@gireeshpunathil
report,doc: define report version semantics
7df1147 
Diagnostics report has a version number representing its format,
yet its rule is not defined. This doc change specifies the rule.

Refs: nodejs/diagnostics#349
Refs: nodejs#28121 (comment)
@gireeshpunathil gireeshpunathil mentioned this pull request Oct 18, 2022
Merged
gireeshpunathil added a commit to gireeshpunathil/node that referenced this pull request Oct 21, 2022
@gireeshpunathil
report,doc: define report version semantics
790a8b2 
Diagnostics report has a version number representing its format,
yet its rule is not defined. This doc change specifies the rule.

Refs: nodejs/diagnostics#349
Refs: nodejs#28121 (comment)
nodejs-github-bot pushed a commit that referenced this pull request Oct 21, 2022
@gireeshpunathil
report,doc: define report version semantics
c127e4e 
Diagnostics report has a version number representing its format,
yet its rule is not defined. This doc change specifies the rule.

Refs: nodejs/diagnostics#349
Refs: #28121 (comment)
PR-URL: #45050
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
@gireeshpunathil @RafaelGSS
report,doc: define report version semantics
d03619d 
Diagnostics report has a version number representing its format,
yet its rule is not defined. This doc change specifies the rule.

Refs: nodejs/diagnostics#349
Refs: #28121 (comment)
PR-URL: #45050
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@gireeshpunathil @RafaelGSS
report,doc: define report version semantics
66e1dc4 
Diagnostics report has a version number representing its format,
yet its rule is not defined. This doc change specifies the rule.

Refs: nodejs/diagnostics#349
Refs: #28121 (comment)
PR-URL: #45050
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@gireeshpunathil @danielleadams
report,doc: define report version semantics
6e5d411 
Diagnostics report has a version number representing its format,
yet its rule is not defined. This doc change specifies the rule.

Refs: nodejs/diagnostics#349
Refs: #28121 (comment)
PR-URL: #45050
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@gireeshpunathil @danielleadams
report,doc: define report version semantics
d9c06d1 
Diagnostics report has a version number representing its format,
yet its rule is not defined. This doc change specifies the rule.

Refs: nodejs/diagnostics#349
Refs: #28121 (comment)
PR-URL: #45050
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@gireeshpunathil @danielleadams
report,doc: define report version semantics
49da5cd 
Diagnostics report has a version number representing its format,
yet its rule is not defined. This doc change specifies the rule.

Refs: nodejs/diagnostics#349
Refs: #28121 (comment)
PR-URL: #45050
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau left review comments

@boneskull boneskull boneskull left review comments

@sam-github sam-github sam-github left review comments

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. report Issues and PRs related to process.report.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@cjihrig @nodejs-github-bot @boneskull @sam-github @addaleax @richardlau @gireeshpunathil