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

Test for did:dht signed VCs + release #322

Merged
merged 6 commits into from
Nov 29, 2023
Merged

Test for did:dht signed VCs + release #322

merged 6 commits into from
Nov 29, 2023

Conversation

phoebe-lew
Copy link
Contributor

No description provided.

@codesandbox CodeSandbox
Copy link

codesandbox bot commented Nov 29, 2023

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

@phoebe-lew phoebe-lew changed the title Release new version Test for did:dht signed VCs + release Nov 29, 2023
@phoebe-lew phoebe-lew marked this pull request as ready for review November 29, 2023 12:31
decentralgabe
decentralgabe reviewed Nov 29, 2023
View reviewed changes
packages/credentials/tests/verifiable-credential.spec.ts Outdated
signOptions = {
issuerDid : alice.did,
subjectDid : alice.did,
kid : alice.did + '#' + alice.did.split(':')[2],
Copy link
Member

Choose a reason for hiding this comment

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

the key ID should be the ID of the verification method, in this case it's alice.did + #0

decentralgabe
decentralgabe approved these changes Nov 29, 2023
View reviewed changes
Copy link
Member

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

approving pending small comments

@frankhinek
Copy link
Contributor

approving pending small comments

Approving even though all of the tests are failing? 🙃

@decentralgabe
Copy link
Member

@frankhinek if our pre commit hooks work properly then merging is blocked until they pass. the contents of the PR seem correct even if there are minor fixable test issues.

frankhinek
frankhinek suggested changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@frankhinek frankhinek left a comment

Choose a reason for hiding this comment

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

Since we're going to release @web5/api version 0.8.3 later this week per Angie's announcement on Discord the version string in packages/api/package.json should also be set to 0.8.2 for this PR.

packages/agent/package.json Outdated Show resolved Hide resolved
packages/api/package.json Outdated Show resolved Hide resolved
packages/credentials/package.json Outdated Show resolved Hide resolved
packages/dids/package.json Outdated Show resolved Hide resolved
packages/identity-agent/package.json Outdated Show resolved Hide resolved
packages/proxy-agent/package.json Outdated Show resolved Hide resolved
packages/user-agent/package.json Outdated Show resolved Hide resolved
@phoebe-lew @frankhinek @decentralgabe
Apply suggestions from code review
ea2e611 
Co-authored-by: Frank Hinek <[email protected]>
Co-authored-by: Gabe <[email protected]>
@frankhinek
Copy link
Contributor

@frankhinek if our pre commit hooks work properly then merging is blocked until they pass. the contents of the PR seem correct even if there are minor fixable test issues.

Upon reviewing the changes in this PR and the failing tests, I noticed that the updated version of @web5/crypto introduces a breaking change across the monorepo packages. I believe it would be beneficial for us to take a closer look at the test failures and understand their underlying causes before proceeding with approvals. This approach will help us maintain the high quality of our codebase and ensure smooth integration of new changes. Looking forward to collaborating on refining this PR.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 29, 2023 •
edited
Loading

TBDocs Report

✅ No errors or warnings

@web5/api

  • Project entry file: packages/api/src/index.ts

Updated @ 2023-11-29T14:37:40.097Z - Commit: 87b9367

@codecov Codecov
Copy link

codecov bot commented Nov 29, 2023 •
edited
Loading

Codecov Report

Merging #322 (83702e2) into main (7828b69) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #322   +/-   ##
=======================================
  Coverage   91.77%   91.77%           
=======================================
  Files          73       73           
  Lines       15758    15758           
  Branches     1448     1448           
=======================================
  Hits        14462    14462           
  Misses       1270     1270           
  Partials       26       26
Components Coverage Δ
api 96.68% <ø> (ø)
common 95.00% <ø> (ø)
credentials 94.32% <ø> (ø)
crypto 100.00% <ø> (ø)
dids 88.75% <ø> (ø)
agent 88.07% <ø> (ø)
identity-agent 56.81% <ø> (ø)
proxy-agent 58.43% <ø> (ø)
user-agent 55.22% <ø> (ø)

@decentralgabe
Copy link
Member

@frankhinek I understand your perspective, but respectfully disagree. This is the value of having multiple reviewers with different areas of expertise. Our review system worked as desired, as we noticed different issues with the changes, and the changes were not able to be merged without them being addressed (via hooks and your change request).

I have high trust in my colleagues to address requested changes and merge when they feel sufficient review has been completed (and CI passes, as is enforced).

@frankhinek frankhinek self-requested a review November 29, 2023 13:19
@frankhinek
Fix missing dev dependency
83702e2 
Signed-off-by: Frank Hinek <[email protected]>
@frankhinek frankhinek added the testing related to new or existing tests label Nov 29, 2023
@frankhinek
Copy link
Contributor

Note: Validated that all browser tests pass despite flaky GitHub runner / Karma timeouts

@shamilovtim has that issue almost solved here: #316

@phoebe-lew phoebe-lew merged commit 9c7c866 into main Nov 29, 2023
29 of 30 checks passed
@phoebe-lew phoebe-lew deleted the plew.bump-credentials-version branch November 29, 2023 14:49
finn-block pushed a commit that referenced this pull request Mar 19, 2024
@phoebe-lew @frankhinek @finn-block
Test for did:dht signed VCs + release (#322)
9c57789 
Signed-off-by: Frank Hinek <[email protected]>
Co-authored-by: Frank Hinek <[email protected]>
finn-block pushed a commit that referenced this pull request Mar 19, 2024
@phoebe-lew @frankhinek @finn-block
Test for did:dht signed VCs + release (#322)
1a51e92 
Signed-off-by: Frank Hinek <[email protected]>
Co-authored-by: Frank Hinek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KendallWeihe KendallWeihe KendallWeihe approved these changes

@decentralgabe decentralgabe decentralgabe approved these changes

@frankhinek frankhinek frankhinek approved these changes

@csuwildcat csuwildcat Awaiting requested review from csuwildcat csuwildcat is a code owner

@mistermoe mistermoe Awaiting requested review from mistermoe

Assignees
No one assigned
Labels
testing related to new or existing tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@phoebe-lew @frankhinek @decentralgabe @KendallWeihe