update links to permalink context URLs and other minor editorial notes #346

kdenhartog · 2021-10-20T03:11:33Z

closes #310

💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on Nov 16, 2021, 11:41 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.


😭  Sorry, there was an error generating the HTML. Please report this issue!
Specification: https://rawcdn.githack.com/w3c/did-spec-registries/d38283f3775641d19653da263aeed597647c7f67/index.html?isPreview=true&publishDate=2021-11-16
ReSpec version: 28.0.3
File a bug: https://github.com/w3c/respec/
Error: Error: Evaluation failed: Timeout: document.respec.ready didn't resolve in 28235ms.
    at ExecutionContext._evaluateInternal (/u/spec-generator/node_modules/puppeteer/lib/cjs/puppeteer/common/ExecutionContext.js:221:19)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async ExecutionContext.evaluate (/u/spec-generator/node_modules/puppeteer/lib/cjs/puppeteer/common/ExecutionContext.js:110:16)
    at async generateHTML (/u/spec-generator/node_modules/respec/tools/respecDocWriter.js:276:12)
    at async toHTML (/u/spec-generator/node_modules/respec/tools/respecDocWriter.js:92:18)
    at async Object.generate [as respec] (/u/spec-generator/generators/respec.js:14:44)
    at async /u/spec-generator/server.js:203:44

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

OR13 · 2021-10-20T13:55:44Z

@kdenhartog thanks!

index.html

msporny

I'm dismayed that this PR got 2 positive votes from Editors. It has issues, we need to get better about not rubber stamping PRs with issues.

index.html

OR13 · 2021-10-20T17:06:25Z

I'm dismayed that this PR got 2 positive votes from Editors. It has issues, we need to get better about not rubber stamping PRs with issues.

I hear you. I will try to do better next time.

OR13

Please address the change requests.

msporny · 2021-10-20T17:11:04Z

Please address the change requests.

Thank you, and to be clear, I'm always appreciative of the work that you, @kdenhartog, and @mprorock do.

The concern here is that this is a live document, and it was used as a reason to formally object to DID Core, so we should always remember that whatever commits go in are 1) read by decision makers, and 2) we might not remember to fix the minor things... and those sometimes add up over time to give off a bad impression.

mprorock

waiting for requested changes to be comitted

kdenhartog · 2021-10-20T22:16:01Z

I know that my PRs from first go can be rough at times, especially if they're a bit large so I tend to lean on the review process as my own way of reviewing things myself. It gives me a way to rethink things in a new light after some time away from working on stuff. Please be as rigorous as you feel is necessary in your reviews so we can raise the standard bar of good specs in the community as a whole.

As an example, I'm spotting some inconsistencies here that I was even trying to focus on like making www always used in the URL or not. I ignored even the basic bar that I had set for myself in an attempt to get something done, but that doesn't mean it's ready to merge yet if I'm being honest with myself. I'll get back to fixing this in a few days and try to get all of the concerns addressed.

Thanks everyone for your legitimate feedback and reviews!

TallTed · 2021-11-03T15:50:13Z

The concern here is that this is a live document, and it was used as a reason to formally object to DID Core, so we should always remember that whatever commits go in are 1) read by decision makers, and 2) we might not remember to fix the minor things... and those sometimes add up over time to give off a bad impression.

Going forward, we might try to (1) as much as possible, treat each edit/PR as if it were the last, and therefore not leave known issues in place; and/or (2) explicitly log any issue that comes to light while preparing, but cannot be addressed during, the immediate edit/PR, before committing that immediate edit/PR, so such issues don't inadvertently fall off the desk.

OR13 · 2021-11-04T17:22:38Z

change requests must be addressed, please ping objectors for a re-review.

kdenhartog · 2021-11-05T02:33:50Z

Text has been updated now and is ready for re-review.

In accordance with @TallTed good suggestion to open issues if there's things that still need to be updated I've filed 2 issues.

#358
#359

kdenhartog · 2021-11-05T02:35:11Z

Not sure what's going on with the CI build now. Anyone have an idea what's up with it?

msporny · 2021-11-05T14:51:29Z

Not sure what's going on with the CI build now. Anyone have an idea what's up with it?

The W3C Process 2021 change happened last Tuesday, publication tools were updated. There is a bug in the spec production tools. I expect W3C spec-prod folks are on it. /cc @iherman just in case they're not aware that W3C spec prod CI is broken: https://github.com/w3c/did-spec-registries/runs/4112856369?check_suite_focus=true

iherman · 2021-11-09T10:24:14Z

The issue was discussed in a meeting on 2021-11-04

no resolutions were taken

View the transcript

2.4. update links to permalink context URLs and other minor editorial notes (pr did-spec-registries#346)

See github pull request did-spec-registries#346.

Orie Steele: This was open by Kyle, lots of change requests on it, they remain unaddressed, we don't need to cover it -- close pull request or make changes. Someone should ping Kyle..

Addresses issues with outdated links being registered after the DID Core V1 JSON-LD context removed some terms. Reviews provided by the co-authors listed below. Co-authored-by: Orie Steele <name@eCo-authored-by: Orie Steele <orie@transmute.industries> Co-authored-by: Manu Sporny <msporny@digitalbazaar.com> Co-authored-by: David I. Lehn <dlehn@digitalbazaar.com>

kdenhartog · 2021-11-14T20:09:09Z

I'm not sure what's still causing the ci pipeline failures. I've rechecked respec and fixed what I thought was the issue (forgot a closing tag in one of my notes), but it doesn't appear to have fixed the issue. For me locally, I'm not seeing any warnings or errors in respec so I'm not certain what I should be doing to fix this.

In any case, all the comments raised above have been addressed and this is ready for re-review. Pinging the change requesters for their attention back to this:

@msporny @mprorock @OR13

index.html

msporny · 2021-11-14T20:50:39Z

I'm not sure what's still causing the ci pipeline failures. I've rechecked respec and fixed what I thought was the issue (forgot a closing tag in one of my notes), but it doesn't appear to have fixed the issue. For me locally, I'm not seeing any warnings or errors in respec so I'm not certain what I should be doing to fix this.

In any case, all the comments raised above have been addressed and this is ready for re-review. Pinging the change requesters for their attention back to this:

@msporny @mprorock @OR13

Please don't touch anything, I'm doing major surgery on did-spec-registries right now to get it working w/ the latest stuff.

Everything is broken right now, so trying to debug your issue is probably a waste of your time until we get the other stuff fixed first. I'm on it.

Update 2021-11-14: I'm now blocked, I need access to the Settings page for this repo to make progress.

Update 2021-11-15: Everything should be working again.

OR13 · 2021-11-16T16:37:39Z

This PR seems over taken by events, should be split up into smaller items, that are easier to review.

TallTed

LGTM, albeit on a quick review

iherman · 2021-11-16T17:30:53Z

The issue was discussed in a meeting on 2021-11-16

no resolutions were taken

View the transcript

2.2. update links to permalink context URLs and other minor editorial notes (pr did-spec-registries#346)

See github pull request did-spec-registries#346.

Orie Steele: I think this PR made a lot of changes all at once; changes have been requested for some time.
… PR seems dead to me, suggest it gets split up into smaller PRs.
… will comment on issue.

Manu Sporny: kyle said people are ready to re-review, but I broke things.
… feel like I agree w/orie, don't know if this PR could really be ... too much going on in this PR.
… all changes probably need to be made.
… would be OK with doing a re-review and get agreement to merge it.

kdenhartog · 2021-11-16T21:08:57Z

This PR seems over taken by events, should be split up into smaller items, that are easier to review.

I could split this into two PRs, one where the updates made are updating the registry and one where notes are being made, but that seems like it would just prolong the process of getting this through and wouldn't help to logically connect the purpose of the notes to the new URLs being updated. The notes are being put in place to explain that the contexts linked are not what people should actually be using because it will cause problems. In other words all aspects of this PR are logically connected to the issue that was filed in #310

Orie Steele: I think this PR made a lot of changes all at once; changes have been requested for some time.
… PR seems dead to me, suggest it gets split up into smaller PRs.
… will comment on issue.

The PR has been ready for 12 days and hasn't had anyone to review it. I can't do anything about that. Similarly from my understanding of the issues with the CI pipeline this is held up by things outside my control. I did my best to attempt to debug this, but as far as I can tell this is an issue that's been caused by the updated process and not anything related to the content of this PR. The one part I did have in my control was that the PR was open for 14 days and I was unable to get back to this PR to update it since I was focused on internal engineering work.

OR13 · 2021-11-16T22:10:16Z

@kdenhartog I approved the PR, I think we can fix any problems it introduces in future PRs.

kdenhartog · 2021-11-16T23:18:16Z

@kdenhartog I approved the PR, I think we can fix any problems it introduces in future PRs.

Thanks for taking the time to review it. I was able to get it shrunk down in any case so that the notes are shorter as well.

As a side note, For some reason it got signed with my new work account @kdenhartog-mattr though. I think it's because that's the GPG key it got signed with so it attributed the commit. My intention is to keep doing community work through @kdenhartog only to make it easier on permissions switching and stuff.

kdenhartog · 2021-11-16T23:43:34Z

Ok all the requested changes have been addressed and this PR is ready to go (@msporny not sure if you fixed it, but CI pipeline is passing now) or a new issue has been filed for issues that still need to be addressed. I did open #370 as well so we have a way to formally track that issue as well.

msporny · 2021-11-17T13:58:37Z

index.html

@@ -1282,7 +1308,17 @@ <h4>PgpVerificationKey2021</h4>

      <section>
        <h4>RsaVerificationKey2018</h4>
-
+        <p class="issue">


Just for future information, ReSpec has built in tooling to reference Github issues, we should be using those features instead of hand-coding URLs.

Thanks, I'll take a look at the documents to figure out how that was done. I did it this way because that's how I saw another issue was doing it, but I think it was only happening because it was referencing an external repo issue.

msporny · 2021-11-17T13:59:21Z

index.html

@@ -980,7 +1003,7 @@ <h4>ethereumAddress</h4>
                <a href="https://w3id.org/security#ethereumAddress">security-vocab</a>
              </td>
              <td>
-                <a href="https://w3id.org/security/v3-unstable">security-vocab-v3-unstable</a>
+                <a href="https://w3id.org/security/v3-unstable">https://w3id.org/security/v3-unstable</a>


We should stop pointing to the v3-unstable security context across the board... we're moving to suites, we should make that change as rapidly as possible.

If only I could have somehow broken those permanent links... folks would have no choice but to stop using them... sometimes a breaking change is needed to ensure a swift transition.... it you are not caching these things, you really ought not to be using them.

Agreed - I'd like to completely remove v3-unstable usage and not register anything that's unstable going forward. This raises the more broad question though of whether or not it makes sense to even register contexts for these verification method properties since I think that was the most common place v3-unstable had to be used.

msporny · 2021-11-17T14:00:39Z

Substantive (URLs changing), multiple reviews, changes requested and made, no remaining objections, merging.

kdenhartog requested review from msporny and OR13 as code owners October 20, 2021 03:11

OR13 approved these changes Oct 20, 2021

View reviewed changes

mprorock approved these changes Oct 20, 2021

View reviewed changes

OR13 approved these changes Oct 20, 2021

View reviewed changes

davidlehn reviewed Oct 20, 2021

View reviewed changes

index.html Outdated Show resolved Hide resolved

davidlehn reviewed Oct 20, 2021

View reviewed changes

index.html Outdated Show resolved Hide resolved

davidlehn reviewed Oct 20, 2021

View reviewed changes

index.html Outdated Show resolved Hide resolved

davidlehn reviewed Oct 20, 2021

View reviewed changes

index.html Outdated Show resolved Hide resolved

davidlehn reviewed Oct 20, 2021

View reviewed changes

index.html Outdated Show resolved Hide resolved

davidlehn reviewed Oct 20, 2021

View reviewed changes

index.html Outdated Show resolved Hide resolved

davidlehn reviewed Oct 20, 2021

View reviewed changes

index.html Outdated Show resolved Hide resolved

davidlehn reviewed Oct 20, 2021

View reviewed changes

index.html Show resolved Hide resolved

davidlehn reviewed Oct 20, 2021

View reviewed changes

index.html Outdated Show resolved Hide resolved

msporny requested changes Oct 20, 2021

View reviewed changes

OR13 self-requested a review October 20, 2021 17:06

OR13 requested changes Oct 20, 2021

View reviewed changes

mprorock requested changes Oct 20, 2021

View reviewed changes

kdenhartog force-pushed the kdh/issue-310 branch from bbfb85f to fd2e579 Compare November 5, 2021 02:27

fix html parsing error that was breaking ci pipeline

d3ca1d9

kdenhartog force-pushed the kdh/issue-310 branch from ed72d50 to d3ca1d9 Compare November 14, 2021 20:04

kdenhartog commented Nov 14, 2021

View reviewed changes

index.html Outdated Show resolved Hide resolved

msporny force-pushed the main branch from 56dcbcb to 1603a06 Compare November 15, 2021 13:39

OR13 approved these changes Nov 16, 2021

View reviewed changes

msporny requested review from msporny and mprorock November 16, 2021 16:38

TallTed approved these changes Nov 16, 2021

View reviewed changes

update notes to reduce size and include issue links

ab9dd4c

add space between link text and paragraph text

d38283f

kdenhartog mentioned this pull request Nov 16, 2021

Remove issue call out for issue 240 in DID Core now that it's been resolved #358

Closed

msporny reviewed Nov 17, 2021

View reviewed changes

msporny approved these changes Nov 17, 2021

View reviewed changes

msporny merged commit 8aca191 into main Nov 17, 2021

msporny deleted the kdh/issue-310 branch November 17, 2021 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update links to permalink context URLs and other minor editorial notes #346

update links to permalink context URLs and other minor editorial notes #346

kdenhartog commented Oct 20, 2021 •

edited by pr-preview bot

OR13 commented Oct 20, 2021

msporny left a comment

OR13 commented Oct 20, 2021

OR13 left a comment

msporny commented Oct 20, 2021 •

edited

mprorock left a comment

kdenhartog commented Oct 20, 2021 •

edited

TallTed commented Nov 3, 2021

OR13 commented Nov 4, 2021

kdenhartog commented Nov 5, 2021

kdenhartog commented Nov 5, 2021

msporny commented Nov 5, 2021 •

edited

iherman commented Nov 9, 2021

2.4. update links to permalink context URLs and other minor editorial notes (pr did-spec-registries#346)

kdenhartog commented Nov 14, 2021

msporny commented Nov 14, 2021 •

edited

OR13 commented Nov 16, 2021

TallTed left a comment

iherman commented Nov 16, 2021

2.2. update links to permalink context URLs and other minor editorial notes (pr did-spec-registries#346)

kdenhartog commented Nov 16, 2021

OR13 commented Nov 16, 2021

kdenhartog commented Nov 16, 2021

kdenhartog commented Nov 16, 2021 •

edited

msporny Nov 17, 2021

kdenhartog Nov 17, 2021

msporny Nov 17, 2021

OR13 Nov 17, 2021

kdenhartog Nov 17, 2021 •

edited

msporny commented Nov 17, 2021

update links to permalink context URLs and other minor editorial notes #346

update links to permalink context URLs and other minor editorial notes #346

Conversation

kdenhartog commented Oct 20, 2021 • edited by pr-preview bot

💥 Error: 500 Internal Server Error 💥

OR13 commented Oct 20, 2021

msporny left a comment

Choose a reason for hiding this comment

OR13 commented Oct 20, 2021

OR13 left a comment

Choose a reason for hiding this comment

msporny commented Oct 20, 2021 • edited

mprorock left a comment

Choose a reason for hiding this comment

kdenhartog commented Oct 20, 2021 • edited

TallTed commented Nov 3, 2021

OR13 commented Nov 4, 2021

kdenhartog commented Nov 5, 2021

kdenhartog commented Nov 5, 2021

msporny commented Nov 5, 2021 • edited

iherman commented Nov 9, 2021

2.4. update links to permalink context URLs and other minor editorial notes (pr did-spec-registries#346)

kdenhartog commented Nov 14, 2021

msporny commented Nov 14, 2021 • edited

OR13 commented Nov 16, 2021

TallTed left a comment

Choose a reason for hiding this comment

iherman commented Nov 16, 2021

2.2. update links to permalink context URLs and other minor editorial notes (pr did-spec-registries#346)

kdenhartog commented Nov 16, 2021

OR13 commented Nov 16, 2021

kdenhartog commented Nov 16, 2021

kdenhartog commented Nov 16, 2021 • edited

msporny Nov 17, 2021

Choose a reason for hiding this comment

kdenhartog Nov 17, 2021

Choose a reason for hiding this comment

msporny Nov 17, 2021

Choose a reason for hiding this comment

OR13 Nov 17, 2021

Choose a reason for hiding this comment

kdenhartog Nov 17, 2021 • edited

Choose a reason for hiding this comment

msporny commented Nov 17, 2021

kdenhartog commented Oct 20, 2021 •

edited by pr-preview bot

msporny commented Oct 20, 2021 •

edited

kdenhartog commented Oct 20, 2021 •

edited

msporny commented Nov 5, 2021 •

edited

msporny commented Nov 14, 2021 •

edited

kdenhartog commented Nov 16, 2021 •

edited

kdenhartog Nov 17, 2021 •

edited